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) вариант, так как оба хороши. Всё зависит являются ли части логического выражения граничными условиями или нет.