Является ли создание новых исключений RuntimeException в недоступном коде плохим стилем?

31

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

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();
    }
}
Сок Помаранчовы
источник
12
это технически достижимо, если rethrowне может на самом деле throwисключение. (что может произойти когда-нибудь, если реализация изменится)
njzk2
34
Кроме того, AssertionError может быть семантически лучшим выбором, чем RuntimeException.
JvR
1
Последний бросок избыточен. Если вы включите findbugs или другие инструменты статического анализа, он пометит эти типы строк для удаления.
Бон Ами
7
Теперь, когда я вижу остальную часть кода, я думаю, что, возможно, вам следует заменить «более опытных разработчиков» на «более старших разработчиков». Code Review был бы рад объяснить, почему.
JvR
3
Я попытался создать гипотетические примеры того, почему исключения ужасны. Но ничто, что я когда-либо делал, не держит свечу к этому.
Пол Дрейпер,

Ответы:

36

Во-первых, спасибо за обновление вашего вопроса и показ нам, что rethrowделает. Таким образом, на самом деле то, что он делает, - это преобразование исключений со свойствами в более мелкозернистые классы исключений. Подробнее об этом позже.

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

Ошибки являются значениями

(Неудивительно, что это хорошо известно в го )

Давайте рассмотрим более простой пример, который я использовал перед вашим редактированием: представьте, что вы что-то регистрируете и создаете исключение оболочки, как в ответе Конрада . Давайте назовем это logAndWrap.

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

private Exception logAndWrap(Exception exception) {
    // or whatever it actually does
    Log.e("Ouch! " + exception.getMessage());
    return new CustomWrapperException(exception);
}

Тогда вы throwявно, и ваш компилятор счастлив:

try {
     return translate(mailManagementService.retrieveUserMailConfiguration(id));
} catch (Exception e) {
     throw logAndWrap(e);
}

Что если вы забудете throw?

Как поясняется в комментарии Joe23 , способ защитного программирования , гарантирующий, что исключение всегда генерируется, состоит в том, чтобы явно делать a throw new CustomWrapperException(exception)в конце logAndWrap, как это делает Guava.Throwables . Таким образом, вы знаете, что будет сгенерировано исключение, и ваш анализатор типов счастлив. Однако ваши пользовательские исключения должны быть непроверенными исключениями, что не всегда возможно. Кроме того, я бы оценил риск того, что разработчик пропустит запись, throwочень низким: разработчик должен забыть об этом, а окружающий метод не должен ничего возвращать, иначе компилятор обнаружит пропущенное возвращение. Это интересный способ борьбы с системой типов, но он работает.

Rethrow

Фактическое также rethrowможет быть записано как функция, но у меня есть проблемы с его текущей реализацией:

  • Есть много бесполезных бросков, таких как Фактически необходимы приведения (см. Комментарии):

    if (e instanceof ServiceUnavailableException) {
        ServiceUnavailableException ex = (ServiceUnavailableException) e;
        rethrow(ex);
    }
  • При выдаче / возврате новых исключений старое отбрасывается; в следующем коде a MailLoginNotAvailableExceptionне позволяет мне узнать, какой логин недоступен, что неудобно; более того, трассировки стека будут неполными:

    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();
        }
    }
  • Почему исходный код не создает эти специализированные исключения? Я подозреваю, что rethrowон используется как слой совместимости между (почтовой) подсистемой и логикой бизнес-процессов (возможно, цель состоит в том, чтобы скрыть детали реализации, такие как выданные исключения, заменив их пользовательскими исключениями). Даже если я согласен , что было бы лучше , чтобы поймать свободный код, как это было предложено в ответе Пит Беккером , я не думаю , что вы будете иметь возможность , чтобы удалить catchи rethrowкоды здесь без серьезных реорганизаций.

CoreDump
источник
1
Слепки не бесполезны. Они необходимы, потому что нет динамической отправки, основанной на типе аргумента. Тип аргумента должен быть известен во время компиляции, чтобы вызвать правильный метод.
Joe23
В вашем logAndWrapметоде вы можете выдать исключение вместо его возврата, но сохранить тип возвращаемого значения (Runtime)Exception. Таким образом, блок catch может остаться таким, каким вы его написали (без броска в недоступном коде). Но у него есть дополнительное преимущество, которое вы не можете забыть throw. Если вы вернете исключение и забудете бросок, это приведет к тому, что исключение будет тихо проглочено. Бросок с возвращаемым типом RuntimeException сделает компилятор счастливым, а также позволит избежать этих ошибок. Вот как Throwables.propagateреализована гуава .
Joe23
@ Joe23 Спасибо за разъяснения по поводу статической отправки. О создании исключений внутри функции: вы имеете в виду, что описываемая вами возможная ошибка - это блок catch, который вызывает, logAndWrapно ничего не делает с возвращенным исключением? Это интересно. Внутри функции, которая должна возвращать значение, компилятор заметил бы, что есть путь без возвращаемого значения, но это полезно для методов, которые ничего не возвращают. Еще раз спасибо.
coredump
1
Это именно то, что я имею в виду. И еще раз подчеркну: это не то, что я придумал и заслужил доверие, а почерпнуло из источника в Гуаве.
Joe23
@ Joe23 Я добавил явную ссылку на библиотеку
coredump
52

Эта rethrow(e);функция нарушает принцип, который гласит, что при нормальных обстоятельствах функция вернется, а при исключительных обстоятельствах функция сгенерирует исключение. Эта функция нарушает этот принцип, вызывая исключение при нормальных обстоятельствах. Это источник всей путаницы.

Компилятор предполагает, что эта функция будет возвращаться при нормальных обстоятельствах, поэтому, насколько компилятор может сказать, выполнение может достигнуть конца retrieveUserMailConfigurationфункции, и в этом случае ошибкой является отсутствие returnоператора. RuntimeException, Брошенной там , как предполагается , чтобы облегчить эту проблему компилятора, но это довольно неуклюжий способ сделать это. Еще один способ предотвратить function must return a valueошибку - добавить return null; //to keep the compiler happyутверждение, но, на мой взгляд, это не менее неуклюже.

Так что лично я бы заменил это:

rethrow(e);

с этим:

report(e); 
throw e;

или, еще лучше, (как предложил coredump ) с этим:

throw reportAndTransform(e);

Таким образом, поток управления станет очевидным для компилятора, поэтому ваш финал throw new RuntimeException("cannot reach here");станет не только избыточным, но фактически даже не компилируемым, так как он будет помечен компилятором как недоступный код.

Это самый элегантный, а на самом деле и самый простой способ выбраться из этой безобразной ситуации.

Майк Накис
источник
1
-1 Предложение разделить функцию на два оператора может привести к ошибкам программирования из-за плохого копирования / вставки; Я также немного обеспокоен тем, что вы отредактировали свой вопрос, предложив throw reportAndTransform(e)ему пару минут после того, как я опубликовал свой собственный ответ. Возможно, вы изменили свой вопрос независимо от этого, и я заранее извиняюсь, если это так, но в противном случае люди обычно делают небольшой кредит.
coredump
4
@coredump Я действительно прочитал ваше предложение и даже прочитал другой ответ, который приписывает вам это предложение, и я вспомнил, что в действительности я использовал именно такую ​​конструкцию в прошлом, поэтому я решил включить ее в свой ответ. Я думал, что это не так важно, чтобы включить атрибуцию, потому что мы не говорим здесь об оригинальной идее, но если вы не согласны с этим, я не хочу вас раздражать, так что атрибуция сейчас, в комплекте со ссылкой. Приветствия.
Майк Накис
Не то чтобы у меня был патент на эту конструкцию, что довольно часто; но представьте, что вы пишете ответ, и вскоре после этого он «усваивается» другим. Так что атрибуция высоко ценится. Спасибо.
coredump
3
Там нет ничего плохого как такового с функцией, которая не возвращает. Это происходит постоянно, например, в системах реального времени. Конечная проблема заключается в недостатке Java в том, что язык анализирует и применяет концепцию правильности языков, и тем не менее язык не предоставляет механизма для того, чтобы как-то комментировать, что функция не возвращает. Тем не менее, есть шаблоны проектирования, которые обходят это, такие как throw reportAndTransform(e). Во многих шаблонах проектирования есть патчи, отсутствуют языковые функции. Это одна из них.
Дэвид Хаммен
2
С другой стороны, многие современные языки достаточно сложны. Превращение каждого шаблона дизайна в базовую языковую функцию еще больше раздувает их. Это хороший пример шаблона проектирования, который не принадлежит базовому языку. Как написано, это хорошо понимается не только компилятором, но и многими другими инструментами, которые должны анализировать код.
MSalters
7

throw, Вероятно , добавили , чтобы обойти «метод должен возвращать значение» ошибку , которая в противном случае имела бы место - данные Java поток анализатор достаточно умен, чтобы понять , что не returnнадо после throw, но не после вашего пользовательского rethrow()метода, и нет @NoReturnаннотации что вы могли бы использовать, чтобы исправить это.

Тем не менее, создание нового исключения в недоступном коде кажется излишним. Я бы просто написал return null, зная, что это, на самом деле, никогда не произойдет.

Килиан Фот
источник
Вы правы. Я проверил, что произойдет, если я закомментирую throw new...строку, и она жалуется на отсутствие возврата. Это плохое программирование? Есть ли соглашение о замене недостижимого оператора возврата? Я оставлю этот вопрос без ответа на некоторое время, чтобы поощрить больше вклада. Тем не менее, спасибо за ваш ответ.
Сок Pomaranczowy
3
@SokPomaranczowy Да, это плохое программирование. Этот rethrow()метод скрывает тот факт, что catchисключение исключается. Я бы не стал делать что-то подобное. Плюс, конечно, на rethrow()самом деле может не получиться выбросить исключение, и в этом случае произойдет что-то еще.
Росс Паттерсон
26
Пожалуйста, не заменяйте исключение на return null. За исключением того, что если кто-то в будущем нарушит код, так что последняя строка каким-то образом станет достижимой, будет сразу ясно, когда возникнет исключение. Если вы вместо этого return null, теперь у вас есть nullзначение, плавающее в вашем приложении, которое не ожидалось. Кто знает, где в приложении NullPointerExceptionвозникнет? Если вам не повезет, и это произойдет после вызова этой функции, будет очень трудно отследить настоящую проблему.
unholysampler
5
@MatthewRead Выполнение кода, предназначенного для недоступности, является серьезной ошибкой, return nullвполне вероятно, что замаскирует ошибку, потому что вы не можете отличить другие случаи, когда она возвращается nullот этой ошибки.
CodesInChaos
4
Кроме того, если функция уже возвращает ноль в некоторых обстоятельствах, и вы также используете нулевое возвращение в этих обстоятельствах, то у пользователей теперь есть два возможных сценария, в которых они могут быть, когда они получают нулевое возвращение. Иногда это не имеет значения, потому что нулевой возврат уже означает «нет объекта, и я не уточняю, почему», поэтому добавление еще одной возможной причины почему не имеет большого значения. Но часто добавление неоднозначности - это плохо, и это, безусловно, означает, что ошибки изначально будут рассматриваться как «определенные обстоятельства», а не сразу как «это сломано». Сбой рано.
Стив Джессоп
6

The

throw new RuntimeException("cannot reach here");

Это утверждение позволяет ЧЕЛОВЕКУ, читающему код, что происходит, поэтому намного лучше, чем, например, возвращать ноль. Это также облегчает отладку, если код изменяется неожиданным образом.

Однако rethrow(e)кажется, что это неправильно! Так что в вашем случае я думаю, что рефакторинг кода - лучший вариант. Посмотрите другие ответы (мне больше всего нравится coredump), чтобы узнать, как разобраться в вашем коде.

Ян
источник
4

Я не знаю, есть ли соглашение.

Во всяком случае, другой трюк будет делать так:

private <T> T rethrow(Exception exception) {
    // or whatever it actually does
    Log.e("Ouch! " + exception.getMessage());
    throw new CustomWrapperException(exception);
}

С учетом этого:

try {
     return translate(mailManagementService.retrieveUserMailConfiguration(id));
} catch (Exception e) {
     return rethrow(e);
}

И никаких искусственных RuntimeExceptionбольше не нужно. Хотя на rethrowсамом деле никогда не возвращает никакого значения, теперь это достаточно для компилятора.

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

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

Стремясь к удобочитаемости, вы можете переименовать rethrowи получить что-то вроде:

} catch (Exception e) {
     return nothingJustRethrow(e);
}
Конрад Моравский
источник
2

Если вы удалите tryблок полностью, то вам не нужно rethrowили throw. Этот код делает то же самое, что и оригинал:

public Configuration retrieveUserMailConfiguration(Long id) throws MailException {
    return translate(mailManagementService.retrieveUserMailConfiguration(id));
}

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

РЕДАКТИРОВАТЬ: я неправильно прочитал rethrow(e)просто повторное исключение e. Если этот rethrowметод на самом деле делает что-то иное, чем перебрасывание исключения, то избавление от него меняет семантику этого метода.

Пит Беккер
источник
Люди будут продолжать верить, что подстановочные знаки, которые ничего не обрабатывают, на самом деле являются обработчиками исключений. Ваша версия игнорирует это ошибочное предположение, не делая вид, что справляется с тем, что не может. Вызывающая функция retrieveUserMailConfiguration () ожидает получить что-то обратно. Если эта функция не может вернуть что-то разумное, она должна быстро и громко провалиться Это как если бы другие ответы не видели проблемы, с catch(Exception)которой является одиозный запах кода.
MSW
1
@msw - Грузовое культовое кодирование.
Пит Беккер,
@msw - с другой стороны, он дает вам возможность установить точку останова.
Пит Беккер,
1
Это как если бы вы не видели проблемы, отбрасывающей целую часть логики. Ваша версия явно не делает то же самое, что и оригинал, который, кстати, уже проваливается быстро и громко. Я бы предпочел писать методы без уловок, подобные тому, который содержится в вашем ответе, но при работе с существующим кодом мы не должны нарушать контракты с другими рабочими частями. Может случиться так, что то, что сделано, rethrowбесполезно (и это не главное), но вы не можете просто избавиться от кода, который вам не нравится, и сказать, что он эквивалентен оригиналу, когда это явно не так. Есть побочные эффекты в пользовательской rethrowфункции.
coredump
1
@ jpmc26 Суть вопроса заключается в исключении «здесь нет», которое может возникнуть по другим причинам, кроме предыдущего блока try / catch. Но я согласен, что блок пахнет подозрительно, и если бы этот ответ изначально был более тщательно сформулирован, он получил бы гораздо больше голосов (кстати, я не отрицал). Последнее редактирование хорошо.
coredump