Govnokodu.Net

Пример рефакторинга «извлечение функции» при подсчёте количества семёрок

Исходный код

function find7(S:string):integer;
var
MyArray : TStringList;
n,i,sum, Resultat : integer;
MyInt : String;
begin
MyArray := TStringList.Create;
Trim(S); n:= 0;
while(Length(s)>0) do begin
Trim(S);
if (Pos(' ',S)=0) then begin
MyArray.Insert(n,Copy(S,1,Length(S)));
break;
end else begin if(Copy(S,1,Pos(' ',S))=' ') then begin
  Delete(S, 1, (Pos(' ',S)));
  end else begin
    MyArray.Insert(n,Copy(S,1,Pos(' ',S)));
    Delete(S,1,(Pos(' ',S)));
    n:=n+1;
    end;
end;
end;
  Resultat := 0;
  for i := 0to MyArray.Count-1 do begin
    MyInt := MyArray.Strings[i]; Sum := 0;
    for n := 1to Length(MyInt) do begin 
      if (MyInt[n]='7') then Sum := Sum + 1;
    end;
    if(Sum>=2) then Resultat := Resultat + 1;
  end;
  Result := Resultat;
end;

Как улучшить исходный код

Форматирование кода

Мне явно не хватает пробелов для удобного чтения кода, особенно отступов — с трудом просматривается структура кода.

function find7(S : string) : integer;
var
  MyArray : TStringList;
  n, i, sum, Resultat : integer;
  MyInt : String;
begin
  MyArray := TStringList.Create;
  Trim(S); 
  n:= 0;
  while (Length(s) > 0) do begin
    Trim(S);
    if (Pos(' ', S) = 0) then begin
      MyArray.Insert(n, Copy(S, 1, Length(S)));
      break;
    end else begin
        if (Copy(S, 1, Pos(' ', S)) = ' ') then begin
          Delete(S, 1, Pos(' ', S));
      end else begin
        MyArray.Insert(n,Copy(S, 1, Pos(' ',S)));
        Delete(S, 1, Pos(' ', S));
        n := n + 1;
      end
    end
  end
  
  Resultat := 0;
  for i := 0 to MyArray.Count - 1 do begin
    MyInt := MyArray.Strings[i]; 
    Sum := 0;
    for n := 1 to Length(MyInt) do begin
      if (MyInt[n] = '7') then Sum := Sum + 1;
    end
    if (Sum >= 2) then Resultat := Resultat + 1;
  end

  Result := Resultat;
end

Разделение ответственностей (разделение знаний)

После форматирования вижу, что алгоритм функции состоит из двух крупных блоков: разбора входной строки, в процессе которого строка превращается в массив, а затем поиск чего-то в полученном массиве. Блоки совершенно самостоятельные, они «знают» о том, как делать две разные не связанные между собой вещи — это железный аргумент в пользу их извлечения в отдельные функции.

function find7(S : string) : integer;
  function parseString(S : string) : TStringList
  var 
    MyArray : TStringList;
    n : integer;
  begin
    MyArray := TStringList.Create;
    Trim(S); 
    n:= 0;
    while (Length(s) > 0) do begin
      Trim(S);
      if (Pos(' ', S) = 0) then begin
        MyArray.Insert(n, Copy(S, 1, Length(S)));
        break;
      end else begin
        if (Copy(S, 1, Pos(' ', S)) = ' ') then begin
          Delete(S, 1, Pos(' ', S));
        end
        else begin
          MyArray.Insert(n, Copy(S, 1, Pos(' ', S)));
          Delete(S, 1, Pos(' ', S));
          n := n + 1;
        end;
      end;
    end;
    Result := MyArray;
  end
  function findSevens(MyArray : TStringList) : integer
  var
    i, Sum, Resultat : integer;
    MyInt : String;
  begin
    Resultat := 0;
    for i := 0 to MyArray.Count - 1 do begin
      MyInt := MyArray.Strings[i]; 
      Sum := 0;
      for n := 1 to Length(MyInt) do begin
        if (MyInt[n] = '7') then Sum := Sum + 1;
      end;
      if (Sum >= 2) then Resultat := Resultat + 1;
    end;
    Result := Resultat;
  end
var
  MyArray : TStringList;

begin
  MyArray = parseString(S);
  Result := findSevens(MyArray);
end;

Алгоритм функции parseString

После краткого анализа кода функции становится понятно, что она делает одну простую вещь: разбивает строку на слова по пробелам.

Это можно сделать разными способами: с помощью функции SplitString с последующим удалением пустых слов, либо вот так, вручную ища пробелы внутри строки и нарезая строку на подстроки по найденным пробелам и удаляя найденные слова и пробелы из исходной строки.

Допустим, мы хотим оставить исходный алгоритм с ручным поиском пробелов. Как можно его ещё улучшить? Тут есть некоторые вещи, без которых можно обойтись: например, можно не хранить размер результирующего массива в переменной n, а вставлять новые слова в начало массива, так как порядок слов в итоге не важен. Можно запоминать позицию очередного пробела в отдельной переменной, чтобы не вызывать так много раз Pos(' ', S).

Не очень понятна необходимость проверки if (Copy(S, 1, Pos(' ', S)) = ' ') — она проверяет наличие пробела в начале строки, но с их поиском и удалением успешно справляется вызов Trim(S) в начале каждой итерации.

Не очень хорошо выглядят два вызова MyArray.Insert — стоит избегать такого дублирования.

В цикле есть два способа выхода: когда перестанет срабатывать условие цикла Length(s) > 0, и когда будет обнаружено, что в оставшейся строке уже нет пробелов if (Pos(' ', S) = 0) — тогда выход произойдёт через break. Желательно оставить только один способ. Поскольку проверять, что исходная строка не пустая, всё равно придётся, то есть смысл оставить только выход при обнаружении, что слово в строке — последнее.

function parseString(S : string) : TStringList
var 
  MyArray : TStringList;
  WordEnd : integer;
begin
  MyArray := TStringList.Create;
  Trim(S); 
  if (Length(S) > 0) then begin
    while (true) do begin
      WordEnd := Pos(' ', S);
      if (WordEnd = 0) then begin
        MyArray.Insert(1, Copy(S, 1, Length(S)));
        break;
      end
      MyArray.Insert(1, Copy(S, 1, WordEnd));
      Delete(S, 1, WordEnd);
      Trim(S);
    end
  end
  Result := MyArray;
end

К сожалению, избавиться от двух вызовов MyArray.Insert в этом варианте не получилось. Попробую изменить тактику.

function parseString(S : string) : TStringList
var 
  MyArray : TStringList;
  WordEnd : integer;
begin
  MyArray := TStringList.Create;
  Trim(S);
  while (Length(S) > 0) dobegin
    WordEnd := Pos(' ', S);
    if (WordEnd = 0) then WordEnd := Length(S);
    MyArray.Insert(1, Copy(S, 1, WordEnd));
    Delete(S, 1, WordEnd);
    Trim(S);
  end
  Result := MyArray;
end

Теперь слова и пробелы удаляются из исходной строки до тех пор, пока она полностью не опустеет — тогда цикл закончится.

Алгоритм поиска семёрок

Здесь мне не нравятся названия переменных: их названия не отражают их смысла. MyInt — это очередное рассматриваемое слово, Sum — это количество семёрок в слове, а вовсе никакая не сумма, Resultat — это количество слов, в которых найдено две и больше семёрок.

Заодно можно скорректировать и название самой функции.

Также в этой функции снова обнаружилиь два знания: как подсчитать количество семёрок в слове, и как подсчитать слова с нужным количеством семёрок.

function countWordsWithTwoSevens(MyWords : TStringList) : integer
  function countSevens(word : string) : integer
  var
    i, sevensCount : integer
  begin
    sevensCount := 0;
    for i := 1to Length(word) do begin
      if (word[i] = '7') then sevensCount := sevensCount + 1;
    end;
    Result := sevensCount;
  end
var
  i, sevensCount, wordsCount : integer;
  MyInt : String;
begin
  wordsCount := 0;
  for i := 0 to MyArray.Count - 1 do begin
    sevensCount := countSevens(MyArray.Strings[i]);
    if (sevensCount >= 2) then wordsCount := wordsCount + 1;
  end
  Result := wordsCount;
end

Теория

Сергей Галанин