суббота, 2 июня 2007 г.

Refactoring: Replace Nested Conditional with Guard Clauses

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


public bool MyFunc()
{
if(IsEnable() &&
TabbedMdiManager != null &&
TabbedMdiManager.ActiveTab != null &&
TabbedMdiManager.ActiveTab.IsSelected &&
(TabbedMdiManager.ActiveTab.Form as BaseForm).Navigate())
{
return true;
}
else
{
return false;
}

}

Ради чего делалась проверка условий, если потом пишем два блока с return загадка.
Поэтому без большой науки я преобразовал код к следующему виду:

Вариант 1.

return (IsEnable()
&& TabbedMdiManager != null
&& TabbedMdiManager.ActiveTab != null
&& TabbedMdiManager.ActiveTab.IsSelected
&& (TabbedMdiManager.ActiveTab.Form as BaseForm).Navigate());

Здесь ВАЖНО: 1) return 2) начинать новую строку && - сразу визуально видно в чём попс.

После мелких раздумий, казалось бы ничего нам не мешает написать так:

Вариант 2.

bool NoMdi = TabbedMdiManager != null;
bool NoActiveTab = TabbedMdiManager.ActiveTab != null;
bool IsSelected = TabbedMdiManager.ActiveTab.IsSelected;
bool DoesNavigate = (TabbedMdiManager.ActiveTab.Form as BaseForm).Navigate();

return InEditMode() && NoMdi && NoActiveTab && IsSelected && DoesNavigate;

Или так:

Вариант 3.

bool x = IsEnablee();
x = x && TabbedMdiManager != null && TabbedMdiManager.ActiveTab != null && TabbedMdiManager.ActiveTab.IsSelected;
x = x && (TabbedMdiManager.ActiveTab.Form as BaseForm).Navigate();
return x;


Но оба вариант (2) и (3) имеют ошибку. Оператор if может завершить вычисление не дойдя до конца логического выражения.
А вот вариант 2 и 3 такое не могут. Соответствено мы получим исключение во время проверки, например в строчке IsSelected.


Но если посмотреть на код с другой стороны. То можно увидеть, что первые 4 проверки являются стороживыми условиями для выполнения функции Navigate().
Соответственно используем метод "Replace Nested Conditional with Guard Clauses" получаем:

Вариант 4.

public bool MyFunc()
{
If( !InEnable() ) return false;
If(TabbedMdiManager == null ) return false;
If(TabbedMdiManager.ActiveTab == null ) return false;
If(!TabbedMdiManager.ActiveTab.IsSelected ) return false;
if(!(TabbedMdiManager.ActiveTab.Form is BaseForm)) return false;

return TabbedMdiManager.ActiveTab.Form.Navigate());
}

Вывод: Вариант 1. Затрудняет отладку -- не понятно, какое условие сработало. Нужно делать дополнительные телодвижения.

вариант 2, 3 - содержат ошибку, которая заложена в if. В исходном коде делается предположение, что все хорошо
знают, что if может и не дойти до конца.

Вариант 3. Вводит новую локальную переменную, тем более накапливающуюся -- это не очень хорошее решение.
Новая локальная переменная без надобности и упрощения только усложняют чтение кода. Теперь мне прийдётся в
голове накапливать результат :)

Вариант 4. К нему нужно привыкнуть. Множественный выход - религиозные войны. Но для проверки входных условий -- очень мощный механизм. Ещё один минус -- важен порядок. Последовательные строки с if...return могут создать ложное предположение, что строчки можно поменять местами.

Вывод: народ проголосовал за вариант 1. Я остался при своём (вариант 4).

А проблема одна: условия связанные и их раздирать по варианту (2) и (3) нельзя.

Полезно:
http://www.refactoring.com/catalog/replaceNestedConditionalWithGuardClauses.html


Кстати, сейчас проголосовало несколько человек за варианты. Если бы не было ошибок, то за

вариант 1 - отдано 2 голоса
вариант 2 - отдано 1,5 голоса
вариант 3 - отдан 1 голос
вариант 4 - 0.5 голоса

я отдал за (2) и (4) вариант, так как оба хороши. Всё зависит являются ли части логического выражения граничными условиями или нет.

2 комментария:

  1. я за четвертый метод, множественный выход, и четкое понимание в случае отладки где, что происходит. Многие мои коллеги, кстати, тоже не выберут четвертый вариант, но это просто из-за природной лени, сделать несколько ретурнов лень, а потом провести в отладке с полчасика не лень :). Один вот только помню был за принципиально одну точку выхода из процедуры, но у него это еще с сишных микроконтроллерных времен.

    ОтветитьУдалить
  2. Ужас. Как я мог отдать 1/2 за второй вариант.

    Однозначно вариант 4 !

    :)

    ОтветитьУдалить