codedokode / pasta

Уроки и черновики для изучающих PHP
760 stars 187 forks source link

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

Open colshrapnel opened 4 years ago

colshrapnel commented 4 years ago

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

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

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

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

colshrapnel commented 4 years ago

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

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

codedokode commented 4 years ago

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

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

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

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

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

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

Убрал.

colshrapnel commented 4 years ago

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

try {
     throw
} catch {
}

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

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

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

kubk commented 4 years ago

@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 commented 4 years ago

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

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

codedokode commented 4 years ago

@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 commented 4 years ago

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

В этом примере, когда 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 commented 4 years ago

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

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

ghost commented 4 years ago

Хочу заметить, что наличие условного 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 - это не антипаттерн. Это уже закладка бомбы с часовым механизмом. Пожалуйста, не пишите так.