Элегантные способы справиться, если (если еще) еще

161

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

if(FileExists(file))
{
    contents = OpenFile(file); // <-- prevents inclusion in if
    if(SomeTest(contents))
    {
        DoSomething(contents);
    }
    else
    {
        DefaultAction();
    }
}
else
{
    DefaultAction();
}
  • Есть ли название для такой логики?
  • Я тоже чересчур ОКР?

Я открыт для злых предложений по коду, хотя бы ради любопытства ...

Benjol
источник
8
@ Эммад Карим: два DefaultActionзвонка нарушают принцип СУХОГО
Abyx
Спасибо за ваш ответ, но я думаю, что все в порядке, за исключением того, что вы не используете try / catch, поскольку могут быть ошибки, которые не возвращают результаты и приводят к аварийному завершению (в зависимости от вашего языка программирования).
NoChance
20
Я думаю, что главная проблема здесь в том, что вы работаете на непоследовательных уровнях абстракции . Более высокий уровень абстракции: make sure I have valid data for DoSomething(), and then DoSomething() with it. Otherwise, take DefaultAction(). Мельчайшие подробности о том, что у вас есть данные для DoSomething (), находятся на более низком уровне абстракции и, следовательно, должны быть в другой функции. Эта функция будет иметь имя на более высоком уровне абстракции, а ее реализация будет на низком уровне. Хорошие ответы ниже решают эту проблему.
Гилад Наор
6
Пожалуйста, укажите язык. Возможные решения, стандартные идиомы и давние культурные нормы различны для разных языков и приведут к разным ответам на ваш вопрос.
Калеб
1
Вы можете сослаться на эту книгу «Рефакторинг: Улучшение дизайна существующего кода». Есть несколько разделов о структуре if-else, действительно полезной практике.
Вакер

Ответы:

96

Распакуйте его в отдельную функцию (метод) и используйте returnоператор:

if(FileExists(file))
{
    contents = OpenFile(file); // <-- prevents inclusion in if
    if(SomeTest(contents))
    {
        DoSomething(contents);
        return;
    }
}

DefaultAction();

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

contents_t get_contents(name_t file)
{
    if(!FileExists(file))
        return null;

    contents = OpenFile(file);
    if(!SomeTest(contents)) // like IsContentsValid
        return null;

    return contents;
}

...

contents = get_contents(file)
contents ? DoSomething(contents) : DefaultAction();

Upd:

Почему бы не исключения, почему OpenFileне выдает исключения IO:
я думаю, что это действительно общий вопрос, а не вопрос о файловом IO. Такие имена, как FileExists, OpenFileмогут вводить в заблуждение, но если заменить их на Fooи Barт. Д., То будет понятнее, что они DefaultActionмогут вызываться так же часто, как и DoSomethingисключительный случай. Петер Тёрёк написал об этом в конце своего ответа

Почему существует троичный условный оператор во втором варианте:
если бы был тег [C ++], я написал бы ifзаявление с объявлением of contentsв его условной части:

if(contents_t contents = get_contents(file))
    DoSomething(contents);
else
    DefaultAction();

Но для других (C-подобных) языков if(contents) ...; else ...;это точно так же, как выражение выражения с тернарным условным оператором, но длиннее. Поскольку основной частью кода была get_contentsфункция, я просто использовал более короткую версию (а также опущенный contentsтип). Во всяком случае, это за пределами этого вопроса.

Abyx
источник
93
+1 для множественных возвратов - когда методы сделаны достаточно маленькими , этот подход работал лучше всего для меня
комнат
Не большой поклонник многократных возвратов, хотя я иногда использую его. Это вполне разумно на чем-то простом, но просто плохо масштабируется. Наш стандарт - избегать этого для всех, кроме сумасшедших простых методов, потому что методы имеют тенденцию расти в размерах больше, чем они уменьшаются.
Брайан Кноблаух
3
Множественные пути возврата могут иметь негативные последствия для производительности в программах на C ++, не позволяя оптимизатору использовать RVO (также NRVO, если каждый путь не возвращает один и тот же объект).
Functastic
Я бы порекомендовал изменить логику второго решения: {если (файл существует) {установить содержимое; if (sometest) {вернуть содержимое; }} return null; } Это упрощает поток и уменьшает количество строк.
Клин
1
Привет, Abyx, я заметил, что ты включил некоторые отзывы из комментариев здесь: спасибо за это. Я вычистил все, что было указано в вашем ответе и других ответах.
56

Если язык программирования, который вы используете (0), выполняет короткое замыкание двоичных сравнений (т.е. если не вызывает, SomeTestесли FileExistsвозвращает false) и (1) присваивание возвращает значение (результат OpenFileприсваивается, contentsа затем это значение передается в качестве аргумента to SomeTest), вы можете использовать что-то вроде следующего, но вам все равно советуют комментировать код, отмечая, что сингл =является преднамеренным.

if( FileExists(file) && SomeTest(contents = OpenFile(file)) )
{
    DoSomething(contents);
}
else
{
    DefaultAction();
}

В зависимости от того, насколько сложным является if, может быть лучше иметь переменную flag (которая разделяет тестирование условий успеха / неудачи с кодом, который обрабатывает ошибку DefaultActionв этом случае)

frozenkoi
источник
Вот как бы я это сделал.
Энтони
13
На ifмой взгляд, довольно грубо вкладывать так много кода в утверждение.
moteutsch
15
Мне, наоборот, нравится такая формулировка «если что-то существует и соответствует этому условию». +1
Горпик
Я тоже! Мне лично не нравится, как люди используют множественные возвраты, некоторые помещения не встречаются. Почему вы не инвертировать это сослагательное наклонение и выполнить свой код , если они будут выполнены?
Клар
«Если что-то существует и соответствует этому условию» - это нормально. «если что-то существует и делает что-то связанное здесь и соответствует этому условию», OTOH, сбивает с толку. Другими словами, я не люблю побочные эффекты в состоянии.
Писквор
26

Более серьезным, чем повторение вызова DefaultAction, является сам стиль, потому что код написан не ортогонально (см. Этот ответ для веских причин написания ортогонально).

Чтобы показать, почему неортогональный код плох, рассмотрим оригинальный пример, когда вводится новое требование, чтобы мы не открывали файл, если он хранится на сетевом диске. Ну, тогда мы могли бы просто обновить код следующим образом:

if(FileExists(file))
{
    if(! OnNetworkDisk(file))
    {
        contents = OpenFile(file); // <-- prevents inclusion in if
        if(SomeTest(contents))
        {
            DoSomething(contents);
        }
        else
        {
            DefaultAction();
        }
    }
    else
    {
        DefaultAction();
    }
}
else
{
    DefaultAction();
}

Но также возникает требование, чтобы мы не открывали большие файлы более 2 ГБ. Ну, мы просто обновим снова:

if(FileExists(file))
{
    if(LessThan2Gb(file))
    {
        if(! OnNetworkDisk(file))
        {
            contents = OpenFile(file); // <-- prevents inclusion in if
            if(SomeTest(contents))
            {
                DoSomething(contents);
            }
            else
            {
                DefaultAction();
            }
        }
        else
        {
            DefaultAction();
        }
    else
    {
        DefaultAction();
    }
}
else
{
    DefaultAction();
}

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

Среди ответов, которые написаны правильно ортогонально, есть второй пример Abyx и ответ Яна Худека , поэтому я не буду повторять это, просто укажу, что добавление двух требований в эти ответы будет просто

if(! LessThan2Gb(file))
    return null;

if(OnNetworkDisk(file))
    return null;

(или goto notexists;вместо return null;), не влияя ни на какой другой код, кроме этих добавленных строк . Например, ортогональный.

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

хловдал
источник
8
+1 для меня. Раннее возвращение помогает избежать анти-паттерна стрелки. См. Codinghorror.com/blog/2006/01/flatening-arrow-code.html и lostechies.com/chrismissal/2009/05/27/… До прочтения об этом шаблоне я всегда подписывался на 1 вход / выход для каждой функции теория из-за того, что меня учили 15 лет или около того назад. Я чувствую, что это делает код намного проще для чтения и, как вы упомянули, более понятным.
Мистер Мус
3
@MrMoose: ваше упоминание антишаблонов со стрелками отвечает на явный вопрос Бенджола: «Есть ли название для такой логики?» Опубликуйте это как ответ, и вы получите мой голос.
outis
Это отличный ответ, спасибо. И @MrMoose: «шаблон анти-стрелки», возможно, отвечает на мою первую пулю, так что да, постите. Я не могу обещать, что приму это, но это заслуживает голосов!
Бенджол
@outis. Благодарю. Я добавил ответ. Антишаблон со стрелками, безусловно, актуален в посте Хловдала, и его пункты охраны хорошо справляются с их обходом. Я не знаю, как вы могли бы ответить на второй пункт пули на это. Я не квалифицирован, чтобы диагностировать это :)
Мистер Мус
4
+1 за «тестовые исключения, а не нормальный случай».
Рой Тинкер
25

Очевидно:

Whatever(Arguments)
{
    if(!FileExists(file))
        goto notexists;
    contents = OpenFile(file); // <-- prevents inclusion in if
    if(!SomeTest(contents))
        goto notexists;
    DoSomething(contents);
    return;
notexists:
    DefaultAction();
}

Вы сказали, что вы открыты даже для злых решений, так что, используя злые счета Гото, нет?

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

Когда у вас есть исключения, их будет легче читать, особенно если вы можете использовать OpenFile и DoSomething, просто генерирующие исключение, если условия не выполняются, поэтому вам вообще не нужны явные проверки. С другой стороны, в C ++, Java и C # создание исключения является медленной операцией, поэтому с точки зрения производительности goto все еще предпочтительнее.


Обратите внимание на «зло»: C ++ FAQ 6.15 определяет «зло» как:

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

И это относится к gotoэтому контексту. Структурированные структуры управления потоком лучше в большинстве случаев, но когда вы попадаете в ситуацию, когда они накапливают слишком много своих зол, например, присваивание по условию, вложение более чем на 3 уровня, дублирование кода или длительные условия, gotoможет просто закончиться быть менее злым.

Jan Hudec
источник
11
Мой курсор нависает над кнопкой «принять» ... просто чтобы высмеять всех пуристов. Ооооооо соблазн: D
Бенджол
2
Да да! Это абсолютно «правильный» способ написания кода. Структура кода теперь выглядит так: «Если ошибка, обработайте ошибку. Нормальное действие. Если ошибка, обработайте ошибку. Нормальное действие», что в точности так и должно быть. Весь «нормальный» код написан только с одним уровнем отступа, в то время как весь код, связанный с ошибками, имеет два уровня отступа. Таким образом, нормальный И НАИБОЛЕЕ ВАЖНЫЙ код занимает наиболее заметное визуальное место, и можно очень быстро и легко читать поток последовательно вниз. Обязательно примите этот ответ.
Хловдал
2
И еще один аспект заключается в том, что код, написанный таким образом, является ортогональным. Например, две строки "if (! FileExists (file)) \ n \ tgoto notexists;" теперь связаны ТОЛЬКО с обработкой этого единственного аспекта ошибки (KISS), и что наиболее важно, он не влияет ни на одну из других строк . В этом ответе stackoverflow.com/a/3272062/23118 перечислено несколько веских причин сохранять код ортогональным.
Хловдал
5
Говоря о злых решениях: я могу получить твое решение без goto:for(;;) { if(!FileExists(file)) break; contents = OpenFile(file); if(!SomeTest(contents)) break; DoSomething(contents); return; } /* broken out */ DefaultAction();
herby
4
@herby: Ваше решение является более злым, чем goto, потому что вы злоупотребляете breakтаким образом, что никто не ожидает, что оно будет злоупотреблено, поэтому у людей, читающих код, будет больше проблем с пониманием того, куда их ведет разрыв, чем с goto, который говорит об этом явно. Кроме того, вы используете бесконечный цикл, который будет запускаться только один раз, что будет довольно странно. К сожалению, do { ... } while(0)он не совсем читабелен, потому что вы видите, что это всего лишь забавный блок, когда вы добираетесь до конца, а C не поддерживает разрыв с другими блоками (в отличие от perl).
Ян Худек
12
function FileContentsExists(file) {
    return FileExists(file) ? OpenFile(file) : null;
}

...

contents = FileContentExists(file);
if(contents && SomeTest(contents))
{
    DoSomething(contents);
}
else
{
    DefaultAction();
}
herby
источник
или перейдите к дополнительному мужчине и создайте дополнительный метод FileExistsAndConditionMet (file) ...
UncleZeiv
@herby SomeTestможет иметь ту же семантику, что и существование файла, если SomeTestпроверяет тип файла, например, проверяет, что .gif действительно GIF-файл.
Абикс
1
Да. Зависит. @ Бенджол знает лучше.
Herby
3
... конечно, я имел в виду "пройти лишнюю милю" ... :)
UncleZeiv
2
То есть с равиоли в конечности , даже я не хожу (и я буду крайним в этом) ... Я думаю , что сейчас это хорошо читаемое рассмотрение contents && f(contents). Две функции, чтобы спасти еще одну ?!
Herby
12

Одна возможность:

boolean handled = false;

if(FileExists(file))
{
    contents = OpenFile(file); // <-- prevents inclusion in if
    if(SomeTest(contents))
    {
        DoSomething(contents);
        handled = true;
    }
}
if (!handled)
{
    DefaultAction();
}

Конечно, это делает код немного более сложным по-другому. Так что это во многом вопрос стиля.

Другой подход будет использовать исключения, например:

try
{
    contents = OpenFile(file); // throws IO exception if file not found
    DoSomething(contents); // calls SomeTest() and throws exception on failure
}
catch(Exception e)
{
    DefaultAction();
    // and the exception should be at least logged...
}

Это выглядит проще, однако это применимо только если

  • мы точно знаем, каких исключений ожидать, и DefaultAction()подходит каждому
  • мы ожидаем, что обработка файла будет успешной, а отсутствующий файл или сбой SomeTest()явно являются ошибочным условием, поэтому целесообразно создать для него исключение.
Péter Török
источник
19
Noooo ~! Это не флаговая переменная, это определенно неправильный путь, потому что это приводит к сложному, трудному для понимания (где-то-флаг-становится-истинным) и трудному для рефакторинга кода.
Абикс
Нет, если вы ограничите его как можно более локальной областью. (function () { ... })()в Javascript, { flag = false; ... }в C-подобном и т. д.
herby
+1 за логику исключения, которая вполне может быть наиболее подходящим решением в зависимости от сценария.
Стивен Джеурис
4
+1 Это взаимное «Нееееет!» смешно. Я думаю, что переменная состояния и досрочный возврат являются разумными в определенных случаях. В более сложных подпрограммах я бы выбрал переменную состояния, потому что вместо того, чтобы добавлять сложность, она действительно делает явную логику. В этом нет ничего плохого.
Grossvogel
1
Это наш предпочтительный формат, где я работаю. Похоже, что двумя основными используемыми параметрами являются «множественный возврат» и «флаговые переменные». Ни один из них, по-видимому, не имеет какого-либо истинного преимущества в среднем, но оба соответствуют определенным обстоятельствам лучше, чем другие. Приходится идти с вашим типичным делом. Просто еще один "Emacs" против "Vi" религиозной войны. :-)
Брайан Кноблаух
11

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

if (WeCanDoSomething(file))
{
   DoSomething(contents);
}
else
{
   DefaultAction();
} 

И это заполняет детали.

boolean WeCanDoSomething(file)
{
    if FileExists(file)
    {
        contents = OpenFile(file);
        return (SomeTest(contents));
    }
    else
    {
        return FALSE;
    }
}
Жанна Пиндар
источник
11

Функции должны делать одно. Они должны делать это хорошо. Они должны это делать только.
Роберт Мартин в « Чистом коде»

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

def processFile(self):
    if self.fileMeetsTest():
        self.doSomething()
    else:
        self.defaultAction()

def fileMeetsTest(self):
    return os.path.exists(self.path) and self.contentsTest()

def contentsTest(self):
    with open(self.path) as file:
        line = file.readline()
        return self.firstLineTest(line)

Когда он говорит, что функции должны делать одно, он имеет в виду одно . processFile()выбирает действие, основанное на результате теста, и это все, что он делает. fileMeetsTest()сочетает в себе все условия теста, и это все, что он делает. contentsTest()передает первую строку firstLineTest(), и это все, что он делает.

Вроде бы много функций, но читается практически как прямой английский:

Чтобы обработать файл, проверьте, соответствует ли он тесту. Если это так, то сделайте что-нибудь. В противном случае выполните действие по умолчанию. Файл соответствует тесту, если он существует, и проходит проверку содержимого. Чтобы проверить содержимое, откройте файл и проверьте первую строку. Тест для первой строки ...

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

Karl Bielefeldt
источник
5
+1 Это хороший совет, но то, что составляет «одну вещь», зависит от текущего уровня абстракции. processFile () - это «одна вещь», но две вещи: fileMeetsTest () и doSomething () или defaultAction (). Я боюсь, что аспект «одно» может сбить с толку новичков, которые априори не понимают концепцию.
Калеб
1
Это хорошая цель ... Это все, что я должен сказать об этом ... ;-)
Брайан Ноблаух
1
Мне не нравится неявная передача аргументов в качестве переменных экземпляра. Вы наполняетесь «бесполезными» переменными экземпляра, и есть много способов испортить ваше состояние и сломать инварианты.
hugomg
@Caleb, ProcessFile () действительно делает одну вещь. Как говорит Карл в своем посте, он использует тест, чтобы решить, какое действие предпринять, и откладывает фактическую реализацию возможностей действия на другие методы. Если бы вы добавили еще много альтернативных действий, критерии единственного назначения для метода все равно были бы выполнены, если в непосредственном методе не было вложенности логики.
С.Робинс
6

Что касается того, что это называется, он может легко превратиться в антишаблон со стрелками по мере того, как ваш код расширяется для удовлетворения дополнительных требований (как показано в ответе на https://softwareengineering.stackexchange.com/a/122625/33922 ) и затем попадает в ловушку огромных участков кода с вложенными условными выражениями, которые напоминают стрелку.

Смотрите ссылки, такие как;

http://codinghorror.com/blog/2006/01/flattening-arrow-code.html

http://lostechies.com/chrismissal/2009/05/27/anti-patterns-and-worst-practices-the-arrowhead-anti-pattern/

Об этом и других анти-паттернах можно найти еще много в Google.

Вот несколько замечательных советов, которые Джефф дает в своем блоге по этому поводу;

1) Заменить условия пунктами охраны.

2) Разложить условные блоки на отдельные функции.

3) Конвертировать отрицательные чеки в положительные чеки

4) Всегда как можно быстрее возвращайтесь из функции.

Посмотрите некоторые комментарии в блоге Джеффа относительно предложений Стива Макконнелса о досрочном возвращении;

«Используйте возврат, когда он улучшает читабельность: в некоторых подпрограммах, когда вы знаете ответ, вы хотите немедленно вернуть его в вызывающую подпрограмму. Если подпрограмма определена таким образом, что она не требует дополнительной очистки после ее обнаруживает ошибку, а не возврат немедленно означает, что вам нужно написать больше кода. "

...

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

Я всегда подписывался на 1 вход / выход на теорию функций из-за того, что меня учили 15 лет назад или около того. Я чувствую, что это делает код намного проще для чтения и, как вы упоминаете, более понятным

Мистер Лось
источник
6

Это соответствует правилам СУХОЙ, no-goto и no-множественный возврат, является масштабируемым и читаемым на мой взгляд:

success = FileExists(file);
if (success)
{
    contents = OpenFile(file);
    success = SomeTest(contents);
}
if (success)
{
    DoSomething(contents);
}
else
{
    DefaultAction();
}
mouviciel
источник
1
Соответствие стандартам не обязательно означает хороший код. В настоящее время я не определился с этим фрагментом кода.
Брайан Кноблаух
это просто заменяет 2 defaultAction (); с 2-мя одинаковыми условиями if и добавляет переменную flag, которая imo намного хуже.
Ryathal
3
Преимущество использования такой конструкции состоит в том, что по мере увеличения числа тестов код не начинает вкладывать больше ifs в другие ifs. Кроме того, код для обработки неудачного case ( DefaultAction()) находится только в одном месте, и в целях отладки код не перемещается по вспомогательным функциям, а добавление точек останова в строки, в successкоторых изменяется переменная, может быстро показать, какие тесты пройдены (выше сработавших). точка останова) и какие из них не были проверены (ниже).
Frozenko
1
Да, мне это нравится, но я думаю, что переименую successв ok_so_far:)
Benjol
Это очень похоже на то, что я делаю, когда (1) процесс очень линейный, когда все идет хорошо, и (2) в противном случае у вас был бы анти-паттерн со стрелкой. Я, однако, стараюсь избегать добавления дополнительной переменной, что обычно легко, если вы думаете с точки зрения предпосылок для следующего шага (который несколько отличается от вопроса о том, не прошел ли предыдущий шаг). Если файл существует, откройте файл. Если файл открыт, прочитайте его содержимое. Если у меня есть содержимое, обработайте его, иначе выполните действие по умолчанию.
Адриан Маккарти
3

Я бы извлек его в отдельный метод, а затем:

if(!FileExists(file))
{
    DefaultAction();
    return;
}

contents = OpenFile(file);
if(!SomeTest(contents))
{
    DefaultAction();
    return;
}

DoSomething(contents);

что также позволяет

if(!FileExists(file))
{
    DefaultAction();
    return Result.FileNotFound;
}

contents = OpenFile(file);
if(!SomeTest(contents))
{
    DefaultAction();
    return Result.TestFailed;
}

DoSomething(contents);
return Result.Success;            

тогда, возможно, вы могли бы удалить DefaultActionвызовы и оставить выполнение DefaultActionдля вызывающего:

Result OurMethod(file)
{
    if(!FileExists(file))
    {
        return Result.FileNotFound;
    }

    contents = OpenFile(file);
    if(!SomeTest(contents))
    {
        return Result.TestFailed;
    }

    DoSomething(contents);
    return Result.Success;            
}

void Caller()
{
    // something, something...

    var result = OurMethod(file);
    // if (result == Result.FileNotFound || result == Result.TestFailed), or just
    if (result != Result.Success)        
    {
        DefaultAction();
    }
}

Мне также нравится подход Жанны Пиндар .

Конрад Моравский
источник
3

Для этого конкретного случая ответ достаточно прост ...

Существует состояние гонки между FileExistsи OpenFile: что произойдет , если файл будет удален?

Единственный разумный способ справиться с этим конкретным случаем - пропустить FileExists:

contents = OpenFile(file);
if (!contents) // open failed
    DefaultAction();
else (SomeTest(contents))
    DoSomething(contents);

Это аккуратно решает эту проблему и делает код чище.

В общем: попробуйте переосмыслить проблему и найти другое решение, которое полностью исключает проблему.

Martin Wickman
источник
2

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

Таким образом, ваш пример может стать:

void DoABunchOfStuff()
{
    if(FileExists(file))
    {
        DoSomethingWithFileContent(file);
        return;
    }

    DefaultAction();
}

void DoSomethingWithFileContent(file)
{        
    var contents = GetFileContents(file)

    if(SomeTest(contents))
    {
        DoSomething(contents);
        return;
    }

    DefaultAction();
}

AReturnType GetFileContents(file)
{
    return OpenFile(file);
}

Лично я не возражаю против использования предложения else , так как оно явно указывает, как должна работать логика, и, таким образом, улучшает читабельность вашего кода. Однако некоторые инструменты для улучшения кода предпочитают упрощать до одного оператора if, чтобы не допустить вложения логики.

S.Robins
источник
2

Случай, показанный в примере кода, обычно сводится к одному ifвыражению. Во многих системах функция открытия файла возвращает недопустимое значение, если файл еще не существует. Иногда это поведение по умолчанию; в других случаях он должен быть указан через аргумент. Это означает, что FileExistsтест можно отменить, что также может помочь в условиях гонки, возникающих в результате удаления файла между тестом на существование и открытием файла.

file = OpenFile(path);
if(isValidFileHandle(file) && SomeTest(file)) {
    DoSomething(file);
} else {
    DefaultAction();
}

Это напрямую не решает проблему смешивания на уровне абстракции, поскольку полностью обходит проблему множественных, несвязанных тестов, хотя отказ от теста на существование файла не является несовместимым с разделением уровней абстракции. Предполагая, что недействительные файловые дескрипторы эквивалентны «false», и файловые дескрипторы закрываются, когда они выходят из области видимости:

OpenFileIfSomething(path:String) : FileHandle {
    file = OpenFile(path);
    if (file && SomeTest(file)) {
        return file;
    }
    return null;
}

...

if ((file = OpenFileIfSomething(path))) {
    DoSomething(file);
} else {
    DefaultAction();
}
outis
источник
2

Я согласен с frozenkoi, однако, для C # в любом случае, я думал, что это поможет следовать синтаксису методов TryParse.

if(FileExists(file) && TryOpenFile(file, out contents))
    DoSomething(contents);
else
    DefaultAction();
bool TryOpenFile(object file, out object contents)
{
    try{
        contents = OpenFile(file);
    }
    catch{
        //something bad happened, computer probably exploded
        return false;
    }
    return true;
}
Biff MaGriff
источник
1

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

if (!ProcessFile(file)) { 
  DefaultAction(); 
}

Программисты на Perl и Ruby пишут processFile(file) || defaultAction()

Теперь иди напиши ProcessFile:

if (FileExists(file)) { 
  contents = OpenFile(file);
  if (SomeTest(contents)) {
    processContents(contents);
    return true;
  }
}
return false;
kevin cline
источник
1

Конечно, вы можете пойти так далеко только в таких сценариях, но вот путь:

interface File<T> {
    function isOK():Bool;
    function getData():T;
}

var appleFile:File<Apple> = appleStorage.get(fileURI);
if (appleFile.isOK())
    eat(file.getData());
else
    cry();

Вы можете хотеть дополнительные фильтры. Затем сделайте это:

var appleFile = appleStorage.get(fileURI, isEdible);
//isEdible is of type Apple->Bool and will be used internally to answer to the isOK call
if (appleFile.isOK())
    eat(file.getData());
else
    cry();

Хотя это также может иметь смысл:

function eat(apple:Apple) {
     if (isEdible(apple)) 
         digest(apple);
     else
         die();
}
var appleFile = appleStorage.get(fileURI);
if (appleFile.isOK())
    eat(appleFile.getData());
else
    cry();

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

back2dos
источник
1

Что не так с очевидным

if(!FileExists(file)) {
    DefaultAction();
    return;
}
contents = OpenFile(file);
if(!SomeTest(contents))
{
    DefaultAction();
    return;
}        
DoSomething(contents);

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

Стив Беннетт
источник
0

Я понимаю, что это старый вопрос, но я заметил образец, который не был упомянут; в основном, установка переменной для последующего определения метода (ов), который вы хотели бы вызвать (за исключением if ... else ...).

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

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

Если вы установите переменную, которая определяет состояние, вы можете иметь много глубоко вложенных параметров и обновлять состояние, когда что-то должно быть (или не должно быть) выполнено.

Это можно использовать, как в примере, заданном в вопросе, где мы проверяем, произошло ли «DoSomething», и, если нет, выполните действие по умолчанию. Или у вас может быть состояние для каждого метода, который вы хотите вызвать, установить, когда это применимо, а затем вызвать соответствующий метод за пределами if ... else ...

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

bool ActionDone = false;

if (Method_1(object_A)) // Test 1
{
    result_A = Method_2(object_A); // Result 1

    if (Method_3(result_A)) // Test 2
    {
        Method_4(result_A); // Action 1
        ActionDone = true;
    }
}

if (!ActionDone)
{
    Method_5(); // Default Action
}
Стив Падмор
источник
0

Чтобы уменьшить вложенный IF:

1 / досрочное возвращение;

2 / сложное выражение (с учетом короткого замыкания)

Итак, ваш пример может быть изменен следующим образом:

if( FileExists(file) && SomeTest(contents = OpenFile(file)) )
{
    DoSomething(contents);
    return;
}
DefaultAction();
DQ_vn
источник
0

Я видел много примеров с «return», которые я тоже использую, но иногда я хочу избежать создания новых функций и использовать вместо этого цикл:

while (1) {
    if (FileExists(file)) {
        contents = OpenFile(file);
        if (SomeTest(contents)) {
           DoSomething(contents);
           break;
        } 
    }
    DefaultAction();
    break;
}

Если вы хотите писать меньше строк или ненавидите бесконечные циклы, как я, вы можете изменить тип цикла на «do ... while (0)» и избежать последнего «разрыва».

XzKto
источник
0

Как насчет этого решения:

content = NULL; //I presume OpenFile returns a pointer 
if(FileExists(file))
    contents = OpenFile(file);
if(content != NULL && SomeTest(contents))
    DoSomething(contents);
else
    DefaultAction();

Я сделал предположение, что OpenFile возвращает указатель, но это также может работать с возвращаемым типом значения, указав не возвращаемое значение по умолчанию (коды ошибок или что-то в этом роде).

Конечно, я не ожидаю каких-либо возможных действий с помощью метода SomeTest для указателя NULL (но вы никогда не узнаете), поэтому это также можно рассматривать как дополнительную проверку для указателя NULL для вызова SomeTest (содержимое).

chedi
источник
0

Понятно, что наиболее элегантным и лаконичным решением является использование макроса препроцессора.

#define DOUBLE_ELSE(CODE) else { CODE } } else { CODE }

Который позволяет вам написать красивый код следующим образом:

if(FileExists(file))
{
    contents = OpenFile(file);
    if(SomeTest(contents))
    {
        DoSomething(contents);
    }
    DOUBLE_ELSE(DefaultAction();)

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

Peter Olson
источник
Для некоторых людей и в некоторых языках макросы препроцессора являются злым кодом :)
Benjol
@ Benjol Вы сказали, что были открыты для злых предложений, нет? ;)
Питер Олсон
да, конечно, это было просто твое "избежать зла" :)
Benjol
4
Это так ужасно, я просто должен был это высказать: D
back2dos
Ширли, ты не серьезно !!!!!!
Джим в Техасе
-1

Поскольку вы спросили из любопытства, и ваш вопрос не помечен конкретным языком (даже если ясно, что вы имели в виду императивные языки), возможно, стоит добавить, что языки, поддерживающие отложенную оценку, допускают совершенно иной подход. В этих языках выражения оцениваются только при необходимости, поэтому вы можете определять «переменные» и использовать их только тогда, когда это имеет смысл. Например, в вымышленном языке с ленивыми let/ inструктурами вы забыли о контроле потока и записи:

let
  contents = ReadFile(file)
in
  if FileExists(file) && SomeTest(contents) 
    DoSomething(contents)
  else 
    DefaultAction()
токланд
источник