Мне поручили поддерживать приложение, написанное некоторое время назад более опытными разработчиками. Я наткнулся на этот кусок кода:
public Configuration retrieveUserMailConfiguration(Long id) throws MailException {
try {
return translate(mailManagementService.retrieveUserMailConfiguration(id));
} catch (Exception e) {
rethrow(e);
}
throw new RuntimeException("cannot reach here");
}
Мне любопытно, если бросок RuntimeException("cannot reach here")
оправдан. Я, вероятно, упускаю что-то очевидное, зная, что этот кусок кода исходит от более опытного коллеги.
РЕДАКТИРОВАТЬ: Вот тело rethrow, что некоторые ответы упоминаются. Я посчитал это не важным в этом вопросе.
private void rethrow(Exception e) throws MailException {
if (e instanceof InvalidDataException) {
InvalidDataException ex = (InvalidDataException) e;
rethrow(ex);
}
if (e instanceof EntityAlreadyExistsException) {
EntityAlreadyExistsException ex = (EntityAlreadyExistsException) e;
rethrow(ex);
}
if (e instanceof EntityNotFoundException) {
EntityNotFoundException ex = (EntityNotFoundException) e;
rethrow(ex);
}
if (e instanceof NoPermissionException) {
NoPermissionException ex = (NoPermissionException) e;
rethrow(ex);
}
if (e instanceof ServiceUnavailableException) {
ServiceUnavailableException ex = (ServiceUnavailableException) e;
rethrow(ex);
}
LOG.error("internal error, original exception", e);
throw new MailUnexpectedException();
}
private void rethrow(ServiceUnavailableException e) throws
MailServiceUnavailableException {
throw new MailServiceUnavailableException();
}
private void rethrow(NoPermissionException e) throws PersonNotAuthorizedException {
throw new PersonNotAuthorizedException();
}
private void rethrow(InvalidDataException e) throws
MailInvalidIdException, MailLoginNotAvailableException,
MailInvalidLoginException, MailInvalidPasswordException,
MailInvalidEmailException {
switch (e.getDetail()) {
case ID_INVALID:
throw new MailInvalidIdException();
case LOGIN_INVALID:
throw new MailInvalidLoginException();
case LOGIN_NOT_ALLOWED:
throw new MailLoginNotAvailableException();
case PASSWORD_INVALID:
throw new MailInvalidPasswordException();
case EMAIL_INVALID:
throw new MailInvalidEmailException();
}
}
private void rethrow(EntityAlreadyExistsException e)
throws MailLoginNotAvailableException, MailEmailAddressAlreadyForwardedToException {
switch (e.getDetail()) {
case LOGIN_ALREADY_TAKEN:
throw new MailLoginNotAvailableException();
case EMAIL_ADDRESS_ALREADY_FORWARDED_TO:
throw new MailEmailAddressAlreadyForwardedToException();
}
}
private void rethrow(EntityNotFoundException e) throws
MailAccountNotCreatedException,
MailAliasNotCreatedException {
switch (e.getDetail()) {
case ACCOUNT_NOT_FOUND:
throw new MailAccountNotCreatedException();
case ALIAS_NOT_FOUND:
throw new MailAliasNotCreatedException();
}
}
java
programming-practices
Сок Помаранчовы
источник
источник
rethrow
не может на самом делеthrow
исключение. (что может произойти когда-нибудь, если реализация изменится)Ответы:
Во-первых, спасибо за обновление вашего вопроса и показ нам, что
rethrow
делает. Таким образом, на самом деле то, что он делает, - это преобразование исключений со свойствами в более мелкозернистые классы исключений. Подробнее об этом позже.Поскольку на самом деле я изначально не ответил на главный вопрос, здесь он звучит так: да, обычно это плохой стиль - генерировать исключения времени выполнения в недоступном коде; вам лучше использовать утверждения или, еще лучше, избегать проблемы. Как уже указывалось, компилятор здесь не может быть уверен, что код никогда не выходит из
try/catch
блока. Вы можете изменить свой код, воспользовавшись тем, что ...Ошибки являются значениями
(Неудивительно, что это хорошо известно в го )
Давайте рассмотрим более простой пример, который я использовал перед вашим редактированием: представьте, что вы что-то регистрируете и создаете исключение оболочки, как в ответе Конрада . Давайте назовем это
logAndWrap
.Вместо того, чтобы генерировать исключение как побочный эффект
logAndWrap
, вы можете позволить ему выполнять свою работу как побочный эффект и заставить его возвращать исключение (по крайней мере, то, которое указано во входных данных). Вам не нужно использовать дженерики, только основные функции:Тогда вы
throw
явно, и ваш компилятор счастлив:Что если вы забудете
throw
?Как поясняется в комментарии Joe23 , способ защитного программирования , гарантирующий, что исключение всегда генерируется, состоит в том, чтобы явно делать a
throw new CustomWrapperException(exception)
в концеlogAndWrap
, как это делает Guava.Throwables . Таким образом, вы знаете, что будет сгенерировано исключение, и ваш анализатор типов счастлив. Однако ваши пользовательские исключения должны быть непроверенными исключениями, что не всегда возможно. Кроме того, я бы оценил риск того, что разработчик пропустит запись,throw
очень низким: разработчик должен забыть об этом, а окружающий метод не должен ничего возвращать, иначе компилятор обнаружит пропущенное возвращение. Это интересный способ борьбы с системой типов, но он работает.Rethrow
Фактическое также
rethrow
может быть записано как функция, но у меня есть проблемы с его текущей реализацией:Есть много бесполезных бросков, таких какФактически необходимы приведения (см. Комментарии):При выдаче / возврате новых исключений старое отбрасывается; в следующем коде a
MailLoginNotAvailableException
не позволяет мне узнать, какой логин недоступен, что неудобно; более того, трассировки стека будут неполными:Почему исходный код не создает эти специализированные исключения? Я подозреваю, что
rethrow
он используется как слой совместимости между (почтовой) подсистемой и логикой бизнес-процессов (возможно, цель состоит в том, чтобы скрыть детали реализации, такие как выданные исключения, заменив их пользовательскими исключениями). Даже если я согласен , что было бы лучше , чтобы поймать свободный код, как это было предложено в ответе Пит Беккером , я не думаю , что вы будете иметь возможность , чтобы удалитьcatch
иrethrow
коды здесь без серьезных реорганизаций.источник
logAndWrap
методе вы можете выдать исключение вместо его возврата, но сохранить тип возвращаемого значения(Runtime)Exception
. Таким образом, блок catch может остаться таким, каким вы его написали (без броска в недоступном коде). Но у него есть дополнительное преимущество, которое вы не можете забытьthrow
. Если вы вернете исключение и забудете бросок, это приведет к тому, что исключение будет тихо проглочено. Бросок с возвращаемым типом RuntimeException сделает компилятор счастливым, а также позволит избежать этих ошибок. Вот какThrowables.propagate
реализована гуава .logAndWrap
но ничего не делает с возвращенным исключением? Это интересно. Внутри функции, которая должна возвращать значение, компилятор заметил бы, что есть путь без возвращаемого значения, но это полезно для методов, которые ничего не возвращают. Еще раз спасибо.Эта
rethrow(e);
функция нарушает принцип, который гласит, что при нормальных обстоятельствах функция вернется, а при исключительных обстоятельствах функция сгенерирует исключение. Эта функция нарушает этот принцип, вызывая исключение при нормальных обстоятельствах. Это источник всей путаницы.Компилятор предполагает, что эта функция будет возвращаться при нормальных обстоятельствах, поэтому, насколько компилятор может сказать, выполнение может достигнуть конца
retrieveUserMailConfiguration
функции, и в этом случае ошибкой является отсутствиеreturn
оператора.RuntimeException
, Брошенной там , как предполагается , чтобы облегчить эту проблему компилятора, но это довольно неуклюжий способ сделать это. Еще один способ предотвратитьfunction must return a value
ошибку - добавитьreturn null; //to keep the compiler happy
утверждение, но, на мой взгляд, это не менее неуклюже.Так что лично я бы заменил это:
с этим:
или, еще лучше, (как предложил coredump ) с этим:
Таким образом, поток управления станет очевидным для компилятора, поэтому ваш финал
throw new RuntimeException("cannot reach here");
станет не только избыточным, но фактически даже не компилируемым, так как он будет помечен компилятором как недоступный код.Это самый элегантный, а на самом деле и самый простой способ выбраться из этой безобразной ситуации.
источник
throw reportAndTransform(e)
ему пару минут после того, как я опубликовал свой собственный ответ. Возможно, вы изменили свой вопрос независимо от этого, и я заранее извиняюсь, если это так, но в противном случае люди обычно делают небольшой кредит.throw reportAndTransform(e)
. Во многих шаблонах проектирования есть патчи, отсутствуют языковые функции. Это одна из них.throw
, Вероятно , добавили , чтобы обойти «метод должен возвращать значение» ошибку , которая в противном случае имела бы место - данные Java поток анализатор достаточно умен, чтобы понять , что неreturn
надо послеthrow
, но не после вашего пользовательскогоrethrow()
метода, и нет@NoReturn
аннотации что вы могли бы использовать, чтобы исправить это.Тем не менее, создание нового исключения в недоступном коде кажется излишним. Я бы просто написал
return null
, зная, что это, на самом деле, никогда не произойдет.источник
throw new...
строку, и она жалуется на отсутствие возврата. Это плохое программирование? Есть ли соглашение о замене недостижимого оператора возврата? Я оставлю этот вопрос без ответа на некоторое время, чтобы поощрить больше вклада. Тем не менее, спасибо за ваш ответ.rethrow()
метод скрывает тот факт, чтоcatch
исключение исключается. Я бы не стал делать что-то подобное. Плюс, конечно, наrethrow()
самом деле может не получиться выбросить исключение, и в этом случае произойдет что-то еще.return null
. За исключением того, что если кто-то в будущем нарушит код, так что последняя строка каким-то образом станет достижимой, будет сразу ясно, когда возникнет исключение. Если вы вместо этогоreturn null
, теперь у вас естьnull
значение, плавающее в вашем приложении, которое не ожидалось. Кто знает, где в приложенииNullPointerException
возникнет? Если вам не повезет, и это произойдет после вызова этой функции, будет очень трудно отследить настоящую проблему.return null
вполне вероятно, что замаскирует ошибку, потому что вы не можете отличить другие случаи, когда она возвращаетсяnull
от этой ошибки.The
Это утверждение позволяет ЧЕЛОВЕКУ, читающему код, что происходит, поэтому намного лучше, чем, например, возвращать ноль. Это также облегчает отладку, если код изменяется неожиданным образом.
Однако
rethrow(e)
кажется, что это неправильно! Так что в вашем случае я думаю, что рефакторинг кода - лучший вариант. Посмотрите другие ответы (мне больше всего нравится coredump), чтобы узнать, как разобраться в вашем коде.источник
Я не знаю, есть ли соглашение.
Во всяком случае, другой трюк будет делать так:
С учетом этого:
И никаких искусственных
RuntimeException
больше не нужно. Хотя наrethrow
самом деле никогда не возвращает никакого значения, теперь это достаточно для компилятора.Он возвращает значение в теории (сигнатура метода), а затем освобождается от этого, поскольку вместо этого выдает исключение.
Да, это может выглядеть странно, но опять же - бросать фантом
RuntimeException
или возвращать нули, которые никогда не будут видны в этом мире - это тоже не совсем красота.Стремясь к удобочитаемости, вы можете переименовать
rethrow
и получить что-то вроде:источник
Если вы удалите
try
блок полностью, то вам не нужноrethrow
илиthrow
. Этот код делает то же самое, что и оригинал:Не позволяйте тому факту, что это исходит от более опытных разработчиков, обмануть вас. Это код гнили, и это происходит постоянно. Просто исправь это.
РЕДАКТИРОВАТЬ: я неправильно прочитал
rethrow(e)
просто повторное исключениеe
. Если этотrethrow
метод на самом деле делает что-то иное, чем перебрасывание исключения, то избавление от него меняет семантику этого метода.источник
catch(Exception)
которой является одиозный запах кода.rethrow
бесполезно (и это не главное), но вы не можете просто избавиться от кода, который вам не нравится, и сказать, что он эквивалентен оригиналу, когда это явно не так. Есть побочные эффекты в пользовательскойrethrow
функции.