Кодирование

Используйте говорящие имена переменных

Описание

Используйте имена переменных, одного взгляда на которые достаточно, чтобы понять, для чего они используются. Помните: программист пишет программу, которую прежде всего должны понимать люди, и он сам в том числе. Не заводите, 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. т.е. при таком подходе мы получаем линейный рост сложности отладки и тестирования, а не экспоненциальный. В реальности даже в небольшом методе комбинаций ветвления значительно больше.

idx blog

Tuesday, June 15, 2010

Гибкий расширяемый код, который легко тестировать. Часть 1.

Кодирование

Используйте говорящие имена переменных

Описание

Плохой код

Хороший код

Не используйте многофункциональные переменные

Описание

Плохой код

Хороший код

Минимизируйте глобальные и совместно используемые данные

Описание

Минимизируйте количество локальных переменных

Описание

Рефакторинг

Плохой код

Хороший код

Избегайте длинных методов

Описание

Рефакторинг

Плохой код

Анализ и рефакторинг

Хороший код

Разбор