Как предотвратить анти-шаблон наконечника стрелы


Я немного путают о том, как лучше выполнить рефакторинг мой код в нечто более читаемым.

рассмотрим этот фрагмент кода:

var foo = getfoo();
if(foo!=null)
{
    var bar = getbar(foo);
    if(bar!=null)
    {
        var moo = getmoo(bar);
        if(moo!=null)
        {
            var cow = getcow(moo);
            ...
        }
    }
}
return;

как вы можете видеть, много вложенных if блоки, необходимые, потому что каждый вложенный if зависит от предыдущих значений.

мне было интересно, как сделать мой код немного чище в этом отношении.

некоторые варианты я думал о себе будет:

  • возврат после каждого предложения if, это означает, что у меня будет несколько мест, где я оставлю свой метод
  • бросить ArgumentNullExceptions, после чего я бы поймал их в конце и поместил оператор return в мое предложение finally (или вне блока try / catch)
  • работа с меткой и goto:

большинство из этих вариантов кажутся мне немного "грязными", поэтому мне было интересно, есть ли хороший способ очистить этот беспорядок, который я создал.

13 51

13 ответов:

Я бы пошел на несколько return заявления. Это делает код легко читаемым и понятным.

Не используйте goto по понятным причинам.

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

рассмотрите возможность инвертирования нулевых проверок в:

var foo = getfoo();
if (foo == null)
{
    return;
}
var bar = getbar(foo);
if (bar == null)
{
    return;
}
...etc

вы можете цепочку выражений. Присвоение возвращает присвоенное значение, поэтому вы можете проверить его результат. Кроме того, вы можете использовать назначенную переменную в следующем выражении.

Как только выражение возвращает false, остальные больше не выполняются, потому что все выражение уже вернет false (из-за and операции).

Так, что-то вроде этого должно работать:

Foo foo; Bar bar; Moo moo; Cow cow;

if( (foo = getfoo()) != null &&
    (bar = getbar(foo)) != null &&
    (moo = getmoo(bar)) != null &&
    (cow = getcow(moo)) != null )
{
  ..
}

это один из немногих сценариев, где это вполне приемлемо (если не желательно) использовать goto.

в таких функциях часто будут выделяться ресурсы или изменения состояния, которые сделаны на полпути, которые необходимо отменить до выхода функции.

обычная проблема с решениями на основе возврата (например, rexcfnghk и Gerrie Schenck) заключается в том, что вы должны помнить, чтобы отменить эти изменения состояния до возвратить. Это приводит к дублированию кода и открывает дверь к тонким ошибкам, особенно в больших функций. не делайте этого.


CERT на самом деле рекомендует структурный подход, основанный на goto.

в частности, обратите внимание на их пример кода, взятый из copy_process на kernel/fork.c ядра Linux. Упрощенный вариант концепции выглядит следующим образом:

    if (!modify_state1(true))
        goto cleanup_none;
    if (!modify_state2(true))
        goto cleanup_state1;
    if (!modify_state3(true))
        goto cleanup_state2;

    // ...

cleanup_state3:
    modify_state3(false);
cleanup_state2:
    modify_state2(false);
cleanup_state1:
    modify_state1(false);
cleanup_none:
    return;

по сути, это просто более читаемым версия кода "arrowhead", которая не использует ненужные отступы или дубликаты кода. Эта концепция может быть легко распространена на все, что лучше всего подходит для вашей ситуации.


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

    FILE *f1 = null;
    FILE *f2 = null;
    void *mem = null;

    if ((f1 = fopen(FILE1, "r")) == null)
        goto cleanup;
    if ((f2 = fopen(FILE2, "r")) == null)
        goto cleanup;
    if ((mem = malloc(OBJSIZE)) == null)
        goto cleanup;

    // ...

cleanup:
    free(mem); // These functions gracefully exit given null input
    close(f2);
    close(f1);
    return;

сначала ваше предложение (возврат после каждого предложения if) является довольно хорошим выходом:

  // Contract (first check all the input)
  var foo = getfoo();

  if (Object.ReferenceEquals(null, foo))
    return; // <- Or throw exception, put assert etc.

  var bar = getbar(foo);

  if (Object.ReferenceEquals(null, bar))
    return; // <- Or throw exception, put assert etc.

  var moo = getmoo(bar);

  if (Object.ReferenceEquals(null, moo))
    return; // <- Or throw exception, put assert etc.

  // Routine: all instances (foo, bar, moo) are correct (not null) and we can work with them
  ...

вторая возможность (в вашем случае) заключается в том, чтобы немного изменить функции getbar (), а также getmoo (), чтобы они возвращали null на нулевом входе, поэтому у вас будет

  var foo = getfoo();
  var bar = getbar(foo); // return null if foo is null
  var moo = getmoo(bar); // return null if bar is null

  if ((foo == null) || (bar == null) || (moo == null))
    return; // <- Or throw exception, put assert(s) etc.    

  // Routine: all instances (foo, bar, moo) are correct (not null)
  ...

третья возможность заключается в том, что в сложных случаях вы можете использовать Null Object Desing Patteren

http://en.wikipedia.org/wiki/Null_Object_pattern

сделай это олдскульно:

var foo;
var bar;
var moo;
var cow;
var failed = false;
failed = failed || (foo = getfoo()) == null;
failed = failed || (bar = getbar(foo)) == null;
failed = failed || (moo = getmoo(bar)) == null;
failed = failed || (cow = getcow(moo)) == null;

гораздо яснее-нет стрелки-и расширяется навсегда.

пожалуйста, не переходите к Dark Side и использовать goto или return.

var foo =                        getFoo();
var bar = (foo == null) ? null : getBar(foo);
var moo = (bar == null) ? null : getMoo(bar);
var cow = (moo == null) ? null : getCow(moo);
if (cow != null) {
  ...
}

Если вы можете изменить материал, который вы вызываете, вы можете изменить его, чтобы никогда не возвращать null, но a NULL-Object вместо.

Это позволит вам полностью потерять все "если".

альтернативой является использование "поддельного" одиночного цикла для управления потоком программ. Я не могу сказать, что рекомендую его, но он определенно лучше выглядит и более читаем, чем arrowhead.

добавление" стадии"," фазы " или sth, как эта переменная может упростить отладку и/или обработку ошибок.

int stage = 0;
do { // for break only, possibly with no indent

var foo = getfoo();
if(foo==null) break;

stage = 1;
var bar = getbar(foo);
if(bar==null) break;

stage = 2;
var moo = getmoo(bar);
if(moo==null) break;

stage = 3;
var cow = getcow(moo);

return 0; // end of non-erroreous program flow
}  while (0); // make sure to leave an appropriate comment about the "fake" while

// free resources if necessary
// leave an error message
ERR("error during stage %d", stage);

//return a proper error (based on stage?)
return ERROR;
try
{
  if (getcow(getmoo(getbar(getfoo()))) == null)
  {
    throw new NullPointerException();
  }
catch(NullPointerException ex)
{
  return; //or whatever you want to do when something is null
}

//... rest of the method

это сохраняет основную логику метода незагроможденной и имеет только один исключительный возврат. Его недостатки заключаются в том, что он может быть медленным, если вам* методы медленным, и трудно сказать в отладчике, какой метод вернул значение null.

ответ Рекса Керра действительно очень хороший.
Если вы можете изменить код, хотя, ответ Йенса Шаудера, вероятно, лучше (Null Object pattern)

Если вы можете сделать пример более конкретным, вы, вероятно, можете получить еще больше ответов
Например, в зависимости от" расположения " методов вы можете иметь что-то вроде:

namespace ConsoleApplication8
{
    using MyLibrary;
    using static MyLibrary.MyHelpers;

    class Foo { }
    class Bar { }
    class Moo { }
    class Cow { }


    internal class Program
    {
        private static void Main(string[] args)
        {
            var cow = getfoo()?.getbar()?.getmoo()?.getcow();
        }
    }
}

namespace MyLibrary
{
    using ConsoleApplication8;
    static class MyExtensions
    {
        public static Cow getcow(this Moo moo) => null;
        public static Moo getmoo(this Bar bar) => null;
        public static Bar getbar(this Foo foo) => null;
    }

    static class MyHelpers
    {
        public static Foo getfoo() => null;
    }
}

странно, никто не упомянул метод цепочки.

Если вы создаете один раз метод цепочки класса

Public Class Chainer(Of R)

    Public ReadOnly Result As R

    Private Sub New(Result As R)
        Me.Result = Result
    End Sub

    Public Shared Function Create() As Chainer(Of R)
        Return New Chainer(Of R)(Nothing)
    End Function

    Public Function Chain(Of S)(Method As Func(Of S)) As Chainer(Of S)
        Return New Chainer(Of S)(Method())
    End Function

    Public Function Chain(Of S)(Method As Func(Of R, S)) As Chainer(Of S)
        Return New Chainer(Of S)(If(Result Is Nothing, Nothing, Method(Result)))
    End Function
End Class

вы можете использовать его везде, чтобы составить любое количество функций в последовательность выполнения для получения результата или ничего (Null)

Dim Cow = Chainer(Of Object).Create.
    Chain(Function() GetFoo()).
    Chain(Function(Foo) GetBar(Foo)).
    Chain(Function(Bar) GetMoo(Bar)).
    Chain(Function(Moo) GetCow(Moo)).
    Result

Это тот случай, когда я бы использовал goto.

ваш пример может быть недостаточно, чтобы подтолкнуть меня к краю, и несколько возвратов лучше, если ваш метод достаточно прост. Но этот шаблон может стать довольно обширным, и вам часто нужен какой-то код очистки в конце. При использовании большинства других ответов здесь, Если я могу, часто единственным разборчивым решением является использование goto ' s.

(когда вы это сделаете, не забудьте поместить все ссылки на этикетку внутри один блок, поэтому любой, кто смотрит на код, знает оба goto ' s и переменные ограничены этой частью кода.)

в Javascript и Java вы можете сделать это:

bigIf:  {

    if (!something)      break bigIf;
    if (!somethingelse)  break bigIf;
    if (!otherthing)     break bigIf;

    // Conditionally do something...
}
// Always do something else...
return;

Javascript и Java не имеют goto ' s, что заставляет меня думать, что другие люди заметили, что в этой ситуации вы do в них нуждается.

исключение будет работать для меня тоже, за исключением try / catch беспорядок вы заставляете на вызывающий код. и, C# помещает трассировку стека на бросок, что замедлит ваш код путь вниз, особенно если он обычно вылетает при первой проверке.