31

I was assigned to maintain an application written some time ago by more skilled developers. I came across this piece of code:

public Configuration retrieveUserMailConfiguration(Long id) throws MailException {
        try {
            return translate(mailManagementService.retrieveUserMailConfiguration(id));
        } catch (Exception e) {
            rethrow(e);
        }
        throw new RuntimeException("cannot reach here");
    }

I'm curious if throwing RuntimeException("cannot reach here") is justified. I'm probably missing something obvious knowing that this piece of code comes from more seasoned colleague.
EDIT: Here is rethrow body that some answers referred to. I deemed it not important in this question.

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
    it is technically reachable, if rethrow fails to actually throw an exception. (which may happen some day, if the implementation changes) – njzk2 Jul 27 '15 at 14:50
  • 34
    As an aside, an AssertionError might be a semantically better choice than RuntimeException. – JvR Jul 27 '15 at 15:40
  • 1
    The final throw is redundant. If you enable findbugs or other static analysis tools, it would flag these types of lines for removal. – Bon Ami Jul 27 '15 at 16:23
  • 7
    Now that I see the rest of the code, I'm thinking that maybe you should change "more skilled developers" into "more senior developers". Code Review would be happy to explain why. – JvR Jul 28 '15 at 12:10
  • 3
    I have tried to create hypothetical examples of why exceptions are terrible. But nothing I've ever done holds a candle to this. – Paul Draper Jul 28 '15 at 20:25
36

First, thanks for udpating your question and showing us what rethrow does. So, in fact, what it does is converting exceptions with properties into more fined-grained classes of exceptions. More on this later.

Since I did not really answer the main question originally, here it goes: yes, it is generally bad style to throw runtime exceptions in unreachable code; you'd better use assertions, or even better, avoid the problem. As already pointed out, the compiler here cannot be sure that the code never walks out of the try/catch block. You can refactor your code by taking advantage that...

Errors are values

(Unsurprisingly, it is well-known in go)

Let's use a simpler example, the one I used before your edit: so imagine that you are logging something and building a wrapper exception like in Konrad's answer. Let's call it logAndWrap.

Instead of throwing the exception as a side-effect of logAndWrap, you could let it do its work as a side-effect and make it return an exception (at least, the one given in input). You don't need to use generics, just basic functions:

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

Then, you throw explicitely, and your compiler is happy:

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

What if you forget to throw?

As explained in Joe23's comment, a defensive programming way to ensure that the exception is always thrown would consists in explicitely doing a throw new CustomWrapperException(exception) at the end of logAndWrap, as it is done by Guava.Throwables. That way, you know that the exception will be thrown, and your type analyzer is happy. However, your custom exceptions need to be unchecked exceptions, which is not always possible. Also, I'd rate the risk of a developper missing to write throw to be very low: the developer must forget it and the surrounding method should not return anything, otherwise the compiler would detect a missing return. This is an interesting way to fight the type system, and it works, though.

Rethrow

The actual rethrow can be written as a function too, but I have problems with its current implementation:

  • There are many useless casts like Casts are in fact required (see comments):

    if (e instanceof ServiceUnavailableException) {
        ServiceUnavailableException ex = (ServiceUnavailableException) e;
        rethrow(ex);
    }
    
  • When throwing/returning new exceptions, the old one is discarded; in the following code, a MailLoginNotAvailableException does not allow me to know which login is not available, which is inconvenient; moreover, stacktraces will be incomplete:

    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();
        }
    }
    
  • Why doesn't the originating code throws those specialized exceptions in the first place? I suspect that rethrow is used as a compatibility layer between a (mailing) subsystem and busineess logic (maybe the intent is to hide implementation details like thrown exceptions by replacing them by custom exceptions). Even if I agree that it would be better to have catch-free code, as suggested in Pete Becker's answer, I don't think you'll have an opportunity to remove the catch and rethrow code here without major refactorings.

  • 1
    The casts are not useless. They are required, because there is no dynamic dispatch based on argument type. The argument type must be known at compile time to call the right method. – Joe23 Jul 29 '15 at 8:51
  • In your logAndWrap method you could throw the exception instead of returning it, but keep the return type (Runtime)Exception. This way the catch block can remain the way you wrote it (without the throw in unreachable code). But it has the additional advantage that you cannot forget the throw. If you return the exception and forget the throw this will lead to the exception being silently swallowed. Throwing while having a return type RuntimeException will make the compiler happy while also avoiding these errors. That is how Guava Throwables.propagate is implemented. – Joe23 Jul 29 '15 at 9:01
  • @Joe23 Thanks for the clarification about static dispatch. About throwing exceptions inside the function: do you mean that the possible error you are describing is a catch block which calls logAndWrap but does nothing with the returned exception? That's interesting. Inside a function that must return a value, the compiler would notice that there is a path with no value being returned, but this is useful for methods which do not return anything. Thanks again. – coredump Jul 29 '15 at 9:14
  • 1
    That's exactly what I ment. And just to stress the point again: It's not something I came up with and take credit for, but gleaned from the Guava source. – Joe23 Jul 29 '15 at 12:05
  • @Joe23 I added an explicit reference to the library – coredump Jul 29 '15 at 12:16
52

This rethrow(e); function violates the principle which says that under normal circumstances, a function will return, while under exceptional circumstances, a function will throw an exception. This function violates this principle by throwing an exception under normal circumstances. That's the source of all of the confusion.

The compiler assumes that this function will return under normal circumstances, so as far as the compiler can tell, execution may reach the end of the retrieveUserMailConfiguration function, at which point it is an error not to have a return statement. The RuntimeException thrown there is supposed to alleviate this concern of the compiler, but it is a rather clunky way of doing it. Another way of preventing the function must return a value error is to add a return null; //to keep the compiler happy statement, but that's equally clunky in my opinion.

So, personally, I would replace this:

rethrow(e);

with this:

report(e); 
throw e;

or, better yet, (as coredump suggested,) with this:

throw reportAndTransform(e);

Thus, the flow of control would be made obvious to the compiler, so your final throw new RuntimeException("cannot reach here"); would become not only redundant, but actually not even compilable, since it would be flagged by the compiler as unreachable code.

That's the most elegant and actually also simplest way of getting out of this ugly situation.

  • 1
    -1 Suggesting to split the function into two statements may introduce programming errors due to bad copy/pasting; I am also slightly worried about the fact that you edited your question to suggest throw reportAndTransform(e) a couple of minutes after I posted my own answer. Maybe you modified you question independantly of it, and I apologize in advance if it is so, but otherwise, giving a little credit is something people usually do. – coredump Jul 27 '15 at 16:39
  • 4
    @coredump I did in fact read your suggestion, and I even read another answer which attributes this suggestion to you, and I remembered that I have actually employed precisely such a construct in the past, so I decided to include it in my answer. I thought that it is not so important so as to include an attribution, because we are not talking about an original idea here, but if you take issue with that, I don't want to aggravate you, so the attribution is now there, complete with a link. Cheers. – Mike Nakis Jul 27 '15 at 17:13
  • It is not that I have a patent for this construct, which is quite common; but imagine you write an answer and not long after, it is "assimilated" by another one. So the attribution is much appreciated. Thank you. – coredump Jul 27 '15 at 17:49
  • 3
    There's nothing wrong per se with a function that does not return. It happens all the time, for example, in real-time systems. The ultimate problem is a flaw in java in that the language analyzes and enforces the languages's concept of correctness and yet the language does not provide a mechanism for somehow annotating that a function does not return. However, there are design patterns that get around this such as throw reportAndTransform(e). Many design patterns are patches are missing language features. This is one of them. – David Hammen Jul 28 '15 at 7:28
  • 2
    On the other hand, many modern languages are complex enough. Turning every design pattern into a core language feature will bloat them even further. This is a good example of a design pattern which doesn't belong in the core language. As written, it's well understood not just by the compiler, but also by many other tools that have to parse code. – MSalters Jul 28 '15 at 12:28
7

The throw was probably added to get around the "method must return a value" error that would otherwise occur - the Java data flow analyser is smart enough to understand that no return is necessary after a throw, but not after your custom rethrow() method, and there is no @NoReturn annotation that you could use to fix this.

Nevertheless, creating a new Exception in unreachable code seems superfluous. I would simply write return null, knowing that it does, in fact, never happen.

  • You are right. I checked what would happen if I comment out the throw new... line and it complains about no return statement. Is it bad programming? Is there any convention about substituting an unreachable return statement? I will leave this question unanswered for a while to encourage more input. Nevertheless thanks for your answer. – Sok Pomaranczowy Jul 27 '15 at 12:57
  • 3
    @SokPomaranczowy Yeah, it's bad programming. That rethrow() method hides the fact that the catch is rethrowing the exception. I would avoid doing something like that. Plus, of course, rethrow() actually might fail to rethrow the exception, in which case something else happens. – Ross Patterson Jul 27 '15 at 13:18
  • 26
    Please don't replace the exception with return null. With the exception, if someone in the future breaks the code so the last line some how becomes reachable, it will be immediately clear when the exception is thrown. If you instead return null, now you have a null value floating around in your application that was not expected. Who knows where in the application the NullPointerException will arise? Unless you get lucky and it is mediately after the call to this function, it will be very hard to track down the real problem. – unholysampler Jul 27 '15 at 14:10
  • 5
    @MatthewRead The execution of code intended to be unreachable is a severe bug, return null is quite likely to mask the bug because you can't distinguish the other cases where it returns null from this bug. – CodesInChaos Jul 28 '15 at 6:53
  • 4
    Furthermore, if the function already returns null in some circumstances, and you also use a null return in this circumstance, then users now have two possible scenarios they might be in when they get a null return. Sometimes this doesn't matter, because null return already means "there's no object and I'm not specifying why", so adding one more possible reason why makes little difference. But often adding ambiguity is bad, and it certainly means that bugs will initially be treated like "certain circumstances" instead of immediately looking like "this is broken". Fail early. – Steve Jessop Jul 28 '15 at 8:48
6

The

throw new RuntimeException("cannot reach here");

statement makes it clear to a PERSON reading the code what is going on, so is a lot better then returning null for example. It also make it easier to debug if the code is changed in an unexpected way.

However rethrow(e) just seems wrong! So in your case I think refactoring the code is a better option. See the other answers (I like coredump's best) for ways to sort your code out.

4

I don't know if there's a convention.

Anyhow, another trick would be to do like so:

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

Allowing for this:

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

And no artificial RuntimeException is needed anymore. Even though rethrow never actually returns any value, it's good enough for the compiler now.

It does return a value in theory (method signature), and then it's exempt from actually doing so as it throws an exception instead.

Yeah, it might look weird, but then again - throwing a phantom RuntimeException, or returning nulls that'll never be seen in this world - that's not exactly a thing of beauty either.

Striving for readability, you could rename rethrow and have something like:

} catch (Exception e) {
     return nothingJustRethrow(e);
}
2

If you remove the try block completely then you don't need the rethrow or the throw. This code does exactly the same thing as the original:

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

Don't let the fact that it comes from more seasoned developers fool you. This is code rot, and it happens all the time. Just fix it.

EDIT: I mis-read rethrow(e) as simply re-throwing the exception e. If that rethrow method actually does something other than re-throw the exception, then getting rid of it does change the semantics of this method.

  • People will continue to believe that wildcard catches which handle nothing are actually exception handlers. Your version disregards that flawed assumption by not pretending to handle that which it can't. The caller of retrieveUserMailConfiguration() expects to get something back. If that function cannot return something reasonable, it should fail fast and loudly. It's as if the other answers saw no issue with catch(Exception) which is an odious code-smell. – msw Jul 27 '15 at 20:50
  • 1
    @msw - Cargo-cult coding. – Pete Becker Jul 27 '15 at 21:00
  • @msw - on the other hand, it does give you a place to set a breakpoint. – Pete Becker Jul 27 '15 at 21:01
  • 1
    It's as-if you see no issue discarding a whole part of the logic. Your version clearly does not do the same thing as the original, which by the way, already fails fast and loudly. I'd prefer to write catch-free methods like the one in your answer, but when working with existing code, we should not break contracts with other working parts. It might be that what is done in rethrow is useless (and this is not the point here), but you can't just get rid of code you don't like and say it is equivalent as the original when it is clearly not. There are side-effects in the custom rethrow function. – coredump Jul 27 '15 at 21:47
  • 1
    @jpmc26 The core of the question is about the "cannot reach here" exception, which could arise for other reasons aside from the preceding try/catch block. But I agree that the block smells fishy, and if this answer was more carefully worded originally, it would have gained much more upvotes (I didn't downvote, btw). The last edit is good. – coredump Jul 28 '15 at 7:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.