Следует ли использовать «другое» в ситуациях, когда поток управления делает его избыточным?

18

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

function doSomething(value) {
  if (check1(value)) {
    return -1;
  }
  else if (check2(value)) {
    return value;
  }
  else {
    return false;
  }
}

Как вы можете видеть, if, else ifи elseутверждения используются в сочетании с returnзаявлением. Это кажется довольно интуитивным для случайного наблюдателя, но я думаю, что было бы более элегантно (с точки зрения разработчика программного обеспечения) отбросить else-s и упростить код следующим образом:

function doSomething(value) {
  if (check1(value)) {
    return -1;
  }
  if (check2(value)) {
    return value;
  }
  return false;
}

Это имеет смысл, так как все, что следует за returnоператором (в той же области видимости), никогда не будет выполнено, делая приведенный выше код семантически равным первому примеру.

Что из перечисленного больше соответствует хорошей практике кодирования? Есть ли недостатки любого метода в отношении читабельности кода?

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

носорог
источник
14
Чем elseменьше проблема, тем больше проблема в том, что вы, очевидно, возвращаете два типа данных из одной функции, что делает API функции непредсказуемым.
Энди
3
E_CLEARLY_NOTADUPE
Кодзиро
3
@DavidPacker: как минимум два; может быть три. -1является числом, falseявляется логическим значением и valueздесь не указывается, поэтому это может быть объект любого типа.
Yay295
1
@ Yay295 Я надеялся, что valueэто целое число. Если это может быть что-то, то это еще хуже.
Энди
@DavidPacker Это очень важный момент, особенно когда он затрагивает вопрос о передовой практике. Я согласен с вами и Yay295, однако примеры кода служат примером для демонстрации несвязанной техники кодирования. Тем не менее, я имею в виду, что это важный вопрос, и его не следует упускать из виду в реальном коде.
носорог

Ответы:

23

Мне нравится тот, без elseи вот почему:

function doSomething(value) {
  //if (check1(value)) {
  //  return -1;
  //}
  if (check2(value)) {
    return value;
  }
  return false;
}

Потому что это не сломало ничего, что не должно было быть.

Ненавижу взаимозависимость во всех ее формах (включая именование функций check2()). Изолировать все, что можно изолировать. Иногда вам нужно, elseно я не вижу этого здесь.

candied_orange
источник
Я обычно предпочитаю это также, по причине, указанной здесь. Однако я обычно помещаю комментарий в конце каждого ifблока, } //elseчтобы при чтении кода было ясно, что единственное предназначение выполнения после ifблока кода - это если условие в ifвыражении было ложным. Без чего-то подобного, особенно если код внутри ifблока становится все более сложным, для тех, кто поддерживает код, может быть неясно, что намерение никогда не было выполнено после ifблока, когда ifусловие истинно.
Макьен
@ Макен, ты поднял хорошую мысль. Один я пытался игнорировать. Я также считаю, что после чего-то должно быть сделано, ifчтобы было ясно, что этот раздел кода готов и структура завершена. Чтобы сделать это, я делаю то, чего не делал здесь (потому что я не хотел отвлекать внимание от основной идеи ОП путем внесения других изменений). Я делаю самое простое, что могу сделать, чтобы добиться всего этого, не отвлекаясь. Я добавляю пустую строку.
candied_orange
27

Я думаю, что это зависит от семантики кода. Если ваши три случая зависят друг от друга, укажите это явно. Это повышает удобочитаемость кода и облегчает понимание для всех остальных. Пример:

if (x < 0) {
    return false;
} else if (x > 0) {
    return true;
} else {
    throw new IllegalArgumentException();
}

Здесь вы явно зависите от стоимости xво всех трех случаях. Если вы опустите последнее else, это будет менее понятно.

Если ваши дела напрямую не зависят друг от друга, пропустите это:

if (x < 0) {
    return false;
}
if (y < 0) {
    return true;
}
return false;

Трудно показать это на простом примере без контекста, но я надеюсь, что вы поняли мою точку зрения.

Андре Станнек
источник
6

Я предпочитаю второй вариант (отдельный ifsс нет else ifи рано return), НО, который до тех пор, пока блоки кода короткие .

Если блоки кода длинные, то лучше их использовать else if, потому что в противном случае у вас не будет четкого понимания нескольких точек выхода кода.

Например:

function doSomething(value) {
  if (check1(value)) {
    // ...
    // imagine 200 lines of code
    // ...
    return -1;
  }
  if (check2(value)) {
    // ...
    // imagine 150 lines of code
    // ...
    return value;
  }
  return false;
}

В этом случае лучше использовать else if, хранить код возврата в переменной и иметь только одно предложение возврата в конце.

function doSomething(value) {
  retVal=0;
  if (check1(value)) {
    // ...
    // imagine 200 lines of code
    // ...
    retVal=-1;
  } else if (check2(value)) {
    // ...
    // imagne 150 lines of code
    retVal=value;
  }
  return retVal;
}

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

Тулаинс Кордова
источник
1
Я согласен с вашей предпосылкой, но пока мы обсуждаем, каким должен быть код, можем ли мы в любом случае не просто согласиться, что блоки кода должны быть короткими? Я думаю, что хуже иметь длинный блок кода, не разбитый на функции, чем использовать другое, если, если бы мне пришлось выбирать.
Кодзиро
1
Любопытный аргумент. returnнаходится в пределах 3 строк, ifтак что это не так уж и отличается, else ifдаже после 200 строк кода. Стиль единственного возврата, который вы вводите здесь, является традицией языка c и других языков, которые не сообщают об ошибках за исключением. Задача состояла в том, чтобы создать единое место для очистки ресурсов. Исключительные языки используют finallyблок для этого. Я предполагаю, что это также создает место для установки точки останова, если ваш отладчик не позволит вам поставить ее на функции, закрывающие фигурную скобку.
candied_orange
4
Мне вообще не нравится это задание retVal. Если вы можете безопасно выйти из функции, сделайте это немедленно, не назначая безопасное значение, которое будет возвращено переменной одного выхода из функции. Когда вы используете единственную точку возврата в действительно длинной функции, я обычно не доверяю другим программистам и заканчиваю тем, что просматривал остальную часть функции для других модификаций возвращаемого значения. Быстро потерпеть неудачу и вернуться рано, среди прочего, два правила, по которым я живу.
Энди
2
На мой взгляд, это еще хуже для длинных блоков. Я пытаюсь выйти из функции как можно раньше, независимо от внешнего контекста.
Энди
2
Я здесь с Дэвидом Пакером. Стиль единого возврата заставляет нас изучать все точки назначения, а также код после точек назначения. Изучение точек выхода в стиле раннего возврата было бы для меня более предпочтительным, длинным или коротким. До тех пор, пока язык позволяет окончательно блокировать.
candied_orange
4

Я использую оба в разных обстоятельствах. В проверке валидации я опускаю else. В потоке управления я использую остальное.

bool doWork(string name) {
  if(name.empty()) {
    return false;
  }

  //...do work
  return true;
}

против

bool doWork(int value) {
  if(value % 2 == 0) {
    doWorkWithEvenNumber(value);
  }
  else {
    doWorkWithOddNumber(value);
  }
}

В первом случае читается больше, как будто вы сначала проверяете все предварительные условия, убираете их с пути, а затем переходите к реальному коду, который хотите запустить. Таким образом, сочный код, который вы действительно хотите запустить, напрямую входит в область действия функции; это то, что функция на самом деле.
Второй случай выглядит так, как будто оба пути являются допустимым кодом для запуска, который так же важен для функции, как и другой, основанный на некотором условии. Как таковые, они принадлежат друг к другу на одинаковых уровнях.

Altainia
источник
0

Единственная ситуация, которую я когда-либо видел, это действительно важно, это код вроде этого:

List<?> toList() {
    if (left != null && right != null) {
        Arrays.asList(merge(left, right));
    }
    if (left != null) {
        return Arrays.asList(left);
    }
    if (right != null) {
        return Arrays.asList(right);
    }
    return Arrays.asList();
}

Здесь явно что-то не так. Проблема в том, что не ясно, returnследует ли добавлять или Arrays.asListудалять. Вы не можете исправить это без более глубокого изучения связанных методов. Вы можете избежать этой неоднозначности, всегда используя полностью исчерпывающие блоки if-else. Эта:

List<?> toList() {
    if (left != null && right != null) {
        Arrays.asList(merge(left, right));
    } else if (left != null) {
        return Arrays.asList(left);
    } else if (right != null) {
        return Arrays.asList(right);
    } else {
        return Arrays.asList();
    }
}

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

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

List<?> toList() {
    if (left == null || right == null) {
        throw new IllegalStateException()
    }
    // ...
    // long method
    // ...
}
Banthar
источник
-2

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

Если у вас есть единственная точка выхода для функции, тогда требуются другие.

var result;
if(check1(value)
{
    result = -1;
}
else if(check2(value))
{
    result = value;
}
else 
{
    result = 0;
}
return result;

На самом деле, давайте идти еще дальше.

var result = 0;
var c1 = check1(value);
var c2 = check2(value);

if(c1)
{
    result = -1;
}
else if(c2)
{
    result = value;
}

return result;

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

Ewan
источник
3
Этот стиль происходит от языков с явным управлением ресурсами. Единственная точка была необходима, чтобы высвободить выделенные ресурсы. В языках с автоматическим управлением ресурсами единая точка возврата не так важна. Вы должны сосредоточиться на сокращении количества и объема переменных.
Banthar
a: в вопросе не указан язык. Б: Я думаю, что вы не правы в этом. Есть много веских причин для однократного возвращения. это уменьшает сложность и улучшает читаемость
Ewan
1
Иногда это уменьшает сложность, иногда это увеличивает ее. См. Wiki.c2.com/?ArrowAntiPattern
Роберт Джонсон
Нет, я думаю, что это всегда уменьшает цикломатическую сложность, см. Мой второй пример. check2 всегда работает
Ewan
ранний возврат - это оптимизация, которая усложняет код путем перемещения некоторого кода в условный код
Ewan
-3

Я предпочитаю опускать лишнее выражение else. Всякий раз, когда я вижу это (в обзоре кода или рефакторинге), я задаюсь вопросом, понимает ли автор поток управления и может ли быть ошибка в коде.

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

Андреас Варберг
источник
3
Хотя я согласен с вами, это не опрос общественного мнения. Пожалуйста, подтвердите свои претензии, иначе избиратели не будут относиться к вам по-доброму.
candied_orange