вторник, 2 сентября 2014 г.

Коротко. О рефакторинге. "С колёс"

Совсем не стремлюсь открыть Америку и тем более чему-то "научить".

Скажем так - "меня попросили сделать review кода".

Поэтому просто дам "пример переписывания кода".

Был код:

procedure TraverseTreeItems(const aTree : TTreeViewItem; var ResultList : TList<TTreeViewItem>);
var
 i : Integer;
begin
 for i := 0 to Pred(aTree.Count)  do
 begin
  if aTree.Items[i].IsChecked
   then ResultList.Add(aTree.Items[i]);

  TraverseTreeItems(aTree.Items[i], ResultList);
 end;
end;

procedure TraverseTree(const aTree: TTreeView; var ResultList : TList<TTreeViewItem>);
var
 i : integer;
begin
 for i := 0 to Pred(aTree.Count) do
 begin
  if aTree.Items[i].IsChecked then
   ResultList.Add(aTree.Items[i]);

  TraverseTreeItems(aTree.Items[i], ResultList);
 end;
end;

Вообще говоря, можно написать так:

type
 TTreeViewItemList = TList<TTtreeViewItem>;

...

procedure TraverseTreeItem(anItem : TTreeViewItem; ResultList : TTreeViewItemList);
var
 i : Integer;
begin
 if anItem.IsChecked then
  ResultList.Add(anItem);
 
 for i := 0 to Pred(anItem.Count)  do
  TraverseTreeItems(anItem.Items[i], ResultList);
end;

procedure TraverseTree(aTree: TTreeView; ResultList : TTreeViewItemList);
var
 i : integer;
begin
 for i := 0 to Pred(aTree.Count) do
  TraverseTreeItem(aTree.Items[i], ResultList);
end;

-- и сделать тест, что оба алгоритма - дают одинаковый результат :-) Рандомный. Как мы когда-то писали. (Про тесты я напишу позже, если конечно интересно)

Что мы тут сделали?

Мы перенесли "ответственность" по проверке aTree.Items[i].IsChecked в anItem.IsChecked (из одного метода в другой).

Т.е. спустили часть одного метода в другой.

Ну и ввели "алиас" - TTreeViewItemList.

Ну и убрали лишние const и var. (Кстати в Delphi не хватает всё же спецификаторов параметров типа constref и varref или constobject и varobject и спецификатора const на метод, особенно getter, ну по аналогии с C++, но правда тогда и mutable может понадобиться)

Вот собственно и всё. Ничего "космического".

Мелочь? Да - мелочь! Но из таких "мелочей" и складывается читабельный, сопровождаемый и тестируемый код.

P.S. Можно ещё generic'и применить. Но надо ли? Для одной строчки.

Update.

P.P.S. кстати к TTreeView и TTreeViewItem можно enumerator'ы привесить через helper'ы
попробуете?

http://programmingmindstream.blogspot.ru/2014/08/for-in.html

Или:

for anItem in TTreeViewEnumerator.Get(aTree) do ...
for anItem in TTreeViewEnumerator.Get(aTree) do ...

Где - TTreeViewEnumerator.Get это class function: TEnumerator с overload

мысль понятна?

Тогда можно будет написать так:

type
 TTreeViewItemList = TList<TTtreeViewItem>;

...

procedure TraverseTreeItem(anItem : TTreeViewItem; ResultList : TTreeViewItemList);
var
 l_Item : TTreeViewItem;
begin
 if anItem.IsChecked then
  ResultList.Add(anItem);
 
 for l_Item in TTreeViewEnumerator.Get(anItem)  do
  TraverseTreeItems(anItem.Items[i], ResultList);
end;

procedure TraverseTree(aTree: TTreeView; ResultList : TTreeViewItemList);
var
 l_Item : TTreeViewItem;
begin
 for l_Item in TTreeViewEnumerator.Get(aTree) do
  TraverseTreeItem(l_Item, ResultList);
end;

- мысль понятна?

6 комментариев:

  1. А я однажды, сказав себе "не фиг выпендриваться", отказался от Pred(Count) в пользу Count - 1.

    С точки зрения генерируемого кода - всё едино (оптимизатор отрабатывает одинаково), а с точки зрения читаемости - второе привычно и более понятно большинству (всё-таки Pred и Succ воспринимаются как функции и "тормозят" чтение... ну дело привычки конечно)

    ОтветитьУдалить
    Ответы
    1. Николай, к чему это всё?

      Вы думаете я не знаю? :-) Таки - знаю.

      Как к теме относится? :-) Ну "рефакторинг".. Наверное...

      Читабельнее? Вопрос - спорный. Сами пишете - "дело привычки". "На вкус и цвет - все фломастеры разные".

      Удалить
    2. Ни к чему. Вы публикуете свои мысли, я комментирую своими. За что глаз зацепился, про то и написал. Обидеть не хотел :)

      Кстати, вот насчёт читаемости, как вам больше нравится:

      if SomeVariable > 0 then
      или
      if 0 < SomeVariable then
      ?

      Удалить
    3. if (0 <= anIndex) AND (anIndex < Pred(Count)) then
      - я ответил?

      У вас кстати ошибка :-)

      Удалить
    4. if SomeVariable > 0 then
      =>
      if 0 <= SomeVariable then

      если уж "глаз зацепился"..

      Удалить
    5. Николай, конечно же я ошибся, а не вы.

      Удалить