Кодирование
Используйте говорящие имена переменных
Описание
Используйте имена переменных, одного взгляда на которые достаточно, чтобы понять, для чего они используются. Помните:
программист пишет программу, которую прежде всего должны понимать люди, и он сам в том числе. Не заводите, 1-2-буквенные имена для переменных имеющих самостоятельный смысл, и, наоборот, используйте простые однобуквенные имена для циклов. Используйте понятные, лаконичные сокращения. Сравните: “ClbMng” и “ClipboardMan” (“ClipboardManager”).
Плохой код
var
S: string;
B: Boolean;
begin
. . .
B := TMetaItemsCollection(TMetaItem(Owner).Collection).Refreshing;
. . .
if B then
Result := Result + ', c.relname';
. . .
S := ' LEFT OUTER JOIN pg_proc p ON p.oid = i.indproc';
Хороший код
var
QueryTail: string;
Refreshing: Boolean;
I: Integer;
begin
. . .
Refreshing := TMetaItemsCollection(TMetaItem(Owner).Collection).Refreshing;
. . .
if Refreshing then
Result := Result + ', c.relname';
. . .
QueryTail := ' LEFT OUTER JOIN pg_proc p ON p.oid = i.indproc';
for I := 1 to 5 do
. . .
Не используйте многофункциональные переменные
Описание
«Одна переменная – одна задача»Плохой код
var
Flag: Boolean;
begin
Flag := Database.Connected;
if Flag then
. . .
Flag := Database.Refreshed;
if Flag then
. . .
end;
Хороший код
var
DBConnected: Boolean;
DBRefreshed: Boolean;
begin
DBConnected := Database.Connected;
if DBConnected then
. . .
DBRefreshed := Database.Refreshed;
if DBRefreshed then
. . .
end;
Минимизируйте глобальные и совместно используемые данные
Описание
Минимизируйте количество локальных переменных
Описание
Большое количество локальных переменных в функции, в большинстве случаев говорит о том, что эта функция берет на себя слишком много обязанностей. Разбейте большую функцию на несколько меньших, несущих самостоятельный смысл, и используйте результат их выполнения вместо локальных переменных.
Рефакторинг
- Выделение метода (Extract Method)
- Встраивание метода (Inline Method)
- Замена временной переменной вызовом метода (Replace Temp with Query)
- Замещение алгоритма (Substitute Algorithm)
Плохой код
procedure TSomeObject.DoSomething();
var
B: Boolean;
begin
. . .
B := False;
if Assigned(Database) then
if Assigned(Database.Tables) then
if Database.Tables.Count > 0 then
B := True;
. . .
if B then // Have tables in database
. . .
if B then // Have tables in database
. . .
end;
Хороший код
procedure TSomeObject.HaveTables(ADb: TDb);
begin
Result :=False;
if Assigned(Database) then
if Assigned(Database.Tables) then
if Database.Tables.Count > 0 then
Result := True;
end;
procedure TSomeObject.DoSomething();
begin
. . .
if HaveTables(Database) then
. . .
if HaveTables(Database) then
. . .
end;
Избегайте длинных методов
Описание
Очень сложно поддерживать код, в котором есть большие методы (функции). Скорее всего такой метод, не тестируется, содержит большое количество локальных переменных и берет на себя слишком много обязанностей. Такой винегрет, будет непонятен не только вашим коллегам – он будет не понятен вам, уже через пару дней. Он также провоцирует дублировать код и никакое комментирование не поможет ему стать более читабельным. В начале своего обучения пользуйтесь правилом «15-20» - пусть ваши методы не содержат более 15-20 строк кода. Исключения составляют спецалгоритмы.
Изучите код метода. Вероятно, из него можно выделить не просто цепочку логически-самостоятельных методов, но и самостоятельный класс.
Рефакторинг
- Выделение метода (Extract Method)
- Замена метода объектом методов (Replace Method with Method Object)
- Замена временной переменной вызовом метода (Replace Temp with Query)
- Выделение класса (Extract Class)
Рассмотрим реальный пример.
Плохой код
function TPgField.ChangeFieldIndexSQL(Value: Boolean; IsPriKey: Boolean;
const OldName: string): string;
var
Ind: TPgIndex;
Tbl: TPgTable;
I: Integer;
begin
Result := '';
Tbl := TPgTable(TPgFields(Collection).Owner);
if not Tbl.Indices.Refreshed then
Tbl.Indices.Refresh;
if Value then // Создать
begin
Ind := TPgIndex.Create(Tbl.Indices);
Ind.Parts.Add.FieldName := Name;
if IsPriKey then
Ind.Primary := True
else
Ind.Unique := True;
Result := Ind.Script;
try
Ind.Rollback;
except
end;
end
else begin // Удалить
for I := 0 to Tbl.Indices.Count - 1 do
begin
if IsPriKey then
begin
if not Tbl.Indices[I].Primary then
Continue;
end
else
if not Tbl.Indices[I].Unique then
Continue;
if (Tbl.Indices[I].Parts.Count = 1) and
(AnsiCompareText(Tbl.Indices[I].Parts[0].FieldName, OldName) = 0) then
begin
Ind := Tbl.Indices[I];
Result := Ind.DropSql(Ind.Name);
Break;
end;
end;
end;
end;
Анализ и рефакторинг
И хотя этот метод не претендует на рекорд по длине и непонятности, он тоже подлежит рефакторингу. Невооруженным глазом видно, что в один метод было записано два. Для понятности код был прокомментирован. Одна часть выполняет «создание», вторая – «удаление». В идеале такой метод вообще не должен существовать, должны быть два, отдельных метода. Однако рефакторинг – это последовательное преобразование кода, не меняющего его поведение. Поэтому на этом этапе мы оставим его.
Для начала выделим два метода, заодно переименуем параметры и локальные переменные в более читаемые.
function TPgField.ChangeFieldIndexSQL(DoCreate: Boolean; IsPrimary: Boolean;
const OldName: string): string;
begin
if DoCreate then
Result := CreateIndexSQL(IsPrimary)
else
Result := DropIndexSQL(IsPrimary)
end;
function TPgField.CreateIndexSQL(IsPrimary: Boolean): string;
var
Index: TPgIndex;
Table: TPgTable;
begin
Table := TPgTable(TPgFields(Collection).Owner);
if not Table.Indices.Refreshed then
Table.Indices.Refresh;
Index := TPgIndex.Create(Table.Indices);
Index.Parts.Add.FieldName := Name;
if IsPrimary then
Index.Primary := True
else
Index.Unique := True;
Result := Index.Script;
try
Index.Rollback;
except
end;
end;
function TPgField.DropIndexSql(IsPrimary: Boolean;
const OldName: string): string;
var
I: Integer;
Index: TPgIndex;
Table: TPgTable;
begin
Table := TPgTable(TPgFields(Collection).Owner);
if not Table.Indices.Refreshed then
Table.Indices.Refresh;
for I := 0 to Table.Indices.Count - 1 do
begin
if IsPrimary then
begin
if not Table.Indices[I].Primary then
Continue;
end
else
if not Table.Indices[I].Unique then
Continue;
if (Table.Indices[I].Parts.Count = 1) and
(AnsiCompareText(Tbl.Indices[I].Parts[0].FieldName, OldName) = 0) then
begin
Index := Tbl.Indices[I];
Result := Index.DropSql(Index.Name);
Break;
end;
end;
end;
Взглянем на этот дублирующийся участок кода:
var
Table: TPgTable;
Table := TPgTable(TPgFields(Collection).Owner);
if not Table.Indices.Refreshed then
Table.Indices.Refresh;
Таблица, переменную на которую мы держим, не используется в коде. На деле, нам нужны два следующих метода: получение списка индексов в таблице, в которой находятся наши поля и обновление этого списка. Правильность того, что весь метод получения скрипта для индексов находится в классе полей, мы, по ходу рефакторинга, тоже поставим под сомнение, но здесь разбирать не будем.
function TPgField.GetIndices: TPgIndices;
begin
Result := TPgTable(TPgFields(Collection).Owner).Indices;
end;
procedure TPgField.RefreshIndices;
begin
if not GetIndices.Refreshed then
GetIndices.Refresh;
end;
Посмотрим как изменились наши функции.
function TPgField.CreateIndexSQL(IsPrimary: Boolean): string;
var
Index: TPgIndex;
begin
RefreshIndices;
Index := TPgIndex.Create(GetIndices);
Index.Parts.Add.FieldName := Name;
if IsPrimary then
Index.Primary := True
else
Index.Unique := True;
Result := Index.Script;
try
Index.Rollback;
except
end;
end;
function TPgField.DropIndexSql(IsPrimary: Boolean;
const OldName: string): string;
var
I: Integer;
begin
RefreshIndices;
for I := 0 to GetIndices.Count - 1 do
begin
if IsPrimary then
begin
if not GetIndices[I].Primary then
Continue;
end
else
if not GetIndices[I].Unique then
Continue;
if (GetIndices[I].Parts.Count = 1) and
(AnsiCompareText(GetIndices[I].Parts[0].FieldName, OldName) = 0) then
begin
Result := GetIndices[I].DropSql(GetIndices[I].Name);
Break;
end;
end;
end;
Ну и наконец, взглянем на это сложное условие, что там происходит очевидным не является:
if IsPrimary then
begin
if not GetIndices[I].Primary then
Continue;
end // если ищем Primary - пропускаем
else
if not GetIndices[I].Unique then
Continue; // если ищем не Primary - пропускаем
if (GetIndices[I].Parts.Count = 1) and
// если не пропустили, и у индекса один Part,
// который содержит одно поле, с нужным, старым именем – это то что нам надо
(AnsiCompareText(GetIndices[I].Parts[0].FieldName, OldName) = 0) then
. . .
Выделим сложное условие в отдельный метод:
function TPgField.IsIndexed(Index: TPgIndex; NeedPrimary: Boolean;
const Name: string): Boolean;
begin
Result := (NeedPrimary and Index.Primary) or
(not NeedPrimary and Index.Unique);
if Result then
Result := (Index.Parts.Count = 1) and
(AnsiCompareText(Index.Parts[0].FieldName, Name) = 0)
end;
Хороший код
function TPgField.ChangeFieldIndexSQL(DoCreate: Boolean; IsPrimary: Boolean;
const OldName: string): string;
begin
if DoCreate then
Result := CreateIndexSQL(IsPrimary)
else
Result := DropIndexSQL(IsPrimary, OldName)
end;
function TPgField.CreateIndexSQL(IsPrimary: Boolean): string;
var
Index: TPgIndex;
begin
RefreshIndices;
Index := TPgIndex.Create(GetIndices);
Index.Parts.Add.FieldName := Name;
if IsPrimary then
Index.Primary := True
else
Index.Unique := True;
Result := Index.Script;
try
Index.Rollback;
except
end;
end;
function TPgField.DropIndexSql(IsPrimary: Boolean;
const OldName: string): string;
var
I: Integer;
begin
RefreshIndices;
for I := 0 to GetIndices.Count - 1 do
if IsIndexed(GetIndices[I], IsPrimary, OldName)
begin
Result := GetIndices[I].DropSql(GetIndices[I].Name);
Break;
end;
end;
function TPgField.GetIndices: TPgIndices;
begin
Result := TPgTable(TPgFields(Collection).Owner).Indices;
end;
procedure TPgField.RefreshIndices;
begin
if not GetIndices.Refreshed then
GetIndices.Refresh;
end;
function TPgField.IsIndexed(Index: TPgIndex; NeedPrimary: Boolean;
const Name: string): Boolean;
begin
Result := (NeedPrimary and Index.Primary) or
(not NeedPrimary and Index.Unique);
if Result then
Result := (Index.Parts.Count = 1) and
(AnsiCompareText(Index.Parts[0].FieldName, Name) = 0)
end;
Разбор
- Можно обратить внимание на увеличение количества кода, по сравнению с изначальным вариантом. Это происходит за счет определения новых функций. Однако тут действует принцип подобный тетрису. Так как получившиеся функции могут быть использованы повторно (а в коде есть участки дублирующие выделенную функциональность), мы уменьшаем дублирование, вместе с тем и количество кода будет уменьшаться.
- Читаемость кода значительно улучшилась (взгляните еще раз на первоначальный вариант). Комментарии к нему излишни.
- Такой код проще отлаживать. Количество возможных ошибок на одну функцию меньше(*). В случае возникновения ошибки подробный пошаговый call stack укажет как и с какими аргументами воспроизвести ошибку.
- Код стал тестируемым, покрыть такой код автоматическими тестами значительно проще.
*Объяснение: для понимания этого факта приведем простой пример. Представим, что в исходной функции было 4 зависимых условия IF THEN. Тогда, чтобы покрыть тестами такую функцию, в идеале, нужно написать 2*2*2*2 = 16 тестов. Тоже самое и при отладке, чтобы найти ошибку в такой функции программист вынужден проверить 16 вариантов ее выполнения. Теперь представим, что мы разбили эту функцию на 4 логически-самостоятельных функции. На каждую такую функцию достаточно написать 2 теста, таким образом всего тестов понадобится 2+2+2+2 = 8. т.е. при таком подходе мы получаем линейный рост сложности отладки и тестирования, а не экспоненциальный. В реальности даже в небольшом методе комбинаций ветвления значительно больше.
