Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Невнятная рекоммендация #17

Open
colshrapnel opened this issue Feb 6, 2020 · 9 comments
Open

Невнятная рекоммендация #17

colshrapnel opened this issue Feb 6, 2020 · 9 comments

Comments

@colshrapnel
Copy link

Не надо располагать try/catch и throw на одном уровне — в этом случае проще написать if

Звучит как бред, поскольку в примерах выше куча исключений бросается на том же уровне.
Пример столь же неясный, как и рекомендация. Что означают троеточия? Наличие других операторов кроме throw? И какая проблема тогда что throw прерывает их выполнение?
Почему нет ясного примера - вот код throw, вот он переписан на if?

Не добавляет ясности и то, что сам совет почему-то в разделе про скрытие ошибок(?)

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

@colshrapnel
Copy link
Author

Это из https://github.com/codedokode/pasta/blob/master/php/exceptions.md

Да, и чтобы два раза не вставать. Надо убрать взаимоисключающие параграфы из раздела про mysqli. Если ты вдруг открыл для себя что либа умеет кидать исключения, то надо уже убрать страдания про то, что она этого не умеет.

@codedokode
Copy link
Owner

Спасибо за замечания.

Про "не надо ставить try/throw на одном уровне" - имеется в виду, внутри одной и той же функции, так как в этом случае проще поставить if и код будет проще. Исключения обычно выкидывают и ловят в разных функциях.

Что означают троеточия?

Какой-то другой код.

Возможно, да, формулировку надо сделать более понятной. Попробовал переписать текст.

Надо убрать взаимоисключающие параграфы из раздела про mysqli. Если ты вдруг открыл для себя что либа умеет кидать исключения, то надо уже убрать страдания про то, что она этого не умеет.

Убрал.

@colshrapnel
Copy link
Author

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

try {
     throw
} catch {
}

но при этом люди начинают бояться писать условный throw типа

try {
    if  (condition)
        throw
    }
    if  (condition)
        throw
    }
    if  (condition)
        throw
    }
} catch {
}

поскольку непонятно, о каком уровне речь.
Вопрос появился из-а вопроса на тостере, где человек не мог взять в толк, что такое эти уровни, и как не надо писать.

@kubk
Copy link
Contributor

kubk commented Feb 21, 2020

@colshrapnel Второй пример и не нужно рекомендовать, это использование исключений для control flow, что является антипаттерном: https://softwareengineering.stackexchange.com/a/189225
Такой код лучше на if'ы переписать. В Webstorm даже инспекция такая по умолчанию включена: https://www.jetbrains.com/help/phpstorm/javascript-exception-used-for-local-control-flow.html

@colshrapnel
Copy link
Author

Если хочется написать что исключения не должны использоваться для управления ходом выполнения, вместо goto, то так и надо написать. Но при этом никаких искусственных ограничений изобретать не стоит. Баян из вложенных ифов - это тоже "антипаттерн". И искусственного кода, в котором есть только throw и catch, тоже в природе не встречается. А есть код, в котором исключение может быть выброшено как внутри вызываемой функции, так и вручную. И это будет вполне законный подход,

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

@codedokode
Copy link
Owner

@colshrapnel Эта рекомендация появилась потому, что я не раз видел в коде начинающих этот антипаттерн. То есть они могли бы использовать просто if/break/return, но (из-за незнания) использовали исключения, усложняя код. В случае с функциями, как правило, можно вынести часть кода (внутри try) в отдельную функцию, заменив throw на return.

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

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

Исключение, как правило, выбрасывается функцией, чтобы сообщить о том, что возникла какая-то неожиданная ситуация, которая не позволяет ей выполнить свою работу. Выбрасывание и ловля исключения в одной и той же функции, как правило, не имеет смысла, и может быть заменена на операторы if/break/return, например:

 // неудачный пример кода
function isValidEmail(string $email): ?string {
    try {
        if ($email === '') {
            throw new LogicException("Необходимо указать email");
        }

        if (!preg_match("/.@./u", $email)) {
            throw new LogicException("Адрес email должен содержать символ @");
        }

        return null;    // ошибок нет
    } catch (\LogicException $e) {
        return $e->getMessage(); // ошибка есть
    }
}

// исправленная версия
function isValidEmail(string $email): ?string {
    if ($email === '') {
        return "Необходимо указать email";
    }

    if (!preg_match("/.@./u", $email)) {
        return "Адрес email должен содержать символ @";
    }

    return null;
}

@colshrapnel
Copy link
Author

Спасибо за конкретный пример.

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

Пустой емейл - это ошибка только для функции отправки почты. А для функции проверки емейла - это нормально. То есть это абьюз именно механизма исключений, а не "уровней".

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

     if (!preg_match("/.@./u", $email)) {
        throw new LogicException("Адрес email должен содержать символ @");
    }

напишем

$this->checkEmailRegexp();

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

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

Например, мы обрабатываем в цикле картинки, и при ошибке в одной из картинок мы не должны прерывать исполнение всего скрипта, а просто залогировать ошибку и двигаться дальше. В этом случае нам поможет catch, чтобы отловить любую ошибку, залогировать её, и двигаться дальше. При этом искусственное ограничение "не бросать на том же уровне" помешает нам реализовать нормальный алгоритм. А всё потому, что здесь исключения используются по назначению - для обработки нештатных ситуаций, в отличие от первого примера. В данном случае результатом работы кода является обработанная картинка. И если нам надо бросить исключение в случае невозможности обработать картинку, то не надо этого стесняться. Точно так же не надо стесняться ловить ошибку, если у нас есть конкретный сценарий её обработки.

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

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

@colshrapnel
Copy link
Author

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

Если в весь код состоит только из условий, внутри которых только throw, и всё это обернуто в try-catch - то да, это будет пример неправильного подхода. Вместо throw / catch в этом правильнее будет использовать return или continue. Хотя опять же, я бы не стал заострять на этом внимание, аоскольку придумать какой-то разумный пример будет сложно, а без примера это опять превратится в искусственное ограничение, которое может быть понятно неправильно.

@ghost
Copy link

ghost commented Apr 1, 2020

Хочу заметить, что наличие условного throw на одном уровне с try...catch свидетельствует как минимум об одном запашке - длинный метод. А то и Large class. А то и God Object
Конструкции вида:

try {  
  //Код получения из БД
  if ($isConnectionFail) throw new Exception();  
  //Код отправки в хранилище
  if ($isNull) throw new Exception();  
  ...  
}  
catch(Exception $ex) {  
...  
}

слишком громоздка и говорит о неудачном проектировании бизнес-модели. В конечном счете, это сложно тестировать.
Когда подмывает написать подобное, стоит сесть и подумать - "а какие задачи исполняет эта функция?". И тогда код станет похож хотя бы на это:

try{  
  $value = getFromDb();  
  setToCache($value);  
  ...  
}  
catch(ConnectionFailException $ex){  
...  
}  
catch(InvalidDataException $ex){  
...  
}

На самом деле и эта конструкция имеет некоторые проблемы другого плана, но это если рассматривать её в рамках ООП. Как пример рефакторинга в нужную сторону сгодится.

Я не обладаю глубокими познаниями синтаксиса PHP, сам на нем не пишу большую часть времени. В официальной документации пишут еще так:

try{  
  try{  
    ...  
  }  
  cathc(...){  
  }  
}  
catch(...){  
  ...   
}

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants