30

Looking through Joshua Bloch's 'Effective Java - Second Edition', I stumbled upon the following code on page 152:

double apply(double x, double y) {
    switch(this) {
        case PLUS:   return x + y;
        case MINUS:  return x - y;
        case TIMES:  return x * y;
        case DIVIDE: return x / y;
    }
    throw new AssertionError("Unknown op: " + this);
}

Now what confuses me is, that the AssertionError is actively thrown. Is that considered good practice? To my understanding assertions are used to not interfeer with the code such that when the java programming is started without assertions enabled and the assert-statements are therefore not executed, the behavior doesn't change. I would be rather confused, if I'd get a AssertionException when I run a program without even having assertions enabled.

Even though I understand that the example case might happen quite often, that you analyze a couple of different options and if it is none of them, you should throw an exception.

So is it good practice to throw an AssertionException here, or would it be better to throw a different one? If so, which one would fit best? Maybe IllegalArgumentException?

Edit for clarification: My question is not about whether we should throw an Errorhere, but if we want to throw an Exception or an Error, which one should it be? And is it good practice to actively throw AssertionErrors? The documentation says Thrown to indicate that an assertion has failed, so I have the feeling that we should not actively throw it. Is that correct?

Second Edit: Clear question: Is it good practice to actively throw an AssertionError, or should that be avoided, even though it is possible? (My guess reading the docs is the latter)

Mathias Bader
  • 3,585
  • 7
  • 39
  • 61
  • 1
    It’s quite opinion-based. For my part I throw `AssertionError`. I think the main reason for not checking assertions in production is the cost of checking the assertion, and in the example in the question I don’t think there is one. I’m on Bloch’s side. – Ole V.V. Dec 25 '16 at 23:00
  • 1
    It's definitely an Error, not an Exception. There is no reasonable scenario where one should attempt to catch the error and continue execution. – Hot Licks Dec 26 '16 at 03:46
  • So to answer the title question from what I understand from the discussion here: Yes, it is ok to actively throw an `AssertionError` in your programm. And if you do so, assertions do not need to be switched on. – Mathias Bader Dec 26 '16 at 10:37
  • @MathiasBader - It's your program, you can do whatever you want. – Hot Licks Jan 01 '17 at 02:34

6 Answers6

26

I would agree with Mr. Bloch here - the alternatives (IllegalArgumentException, IllegalStateException, and UnsupportedOperationException) do not properly convey the severity of the issue, and callers may erroneously try to catch and handle this case. In fact if this line is ever reached the program in question is broken, and the only sane thing to do is exit.

The point here is that enum has a finite set of values, thus it should be impossible to reach the throw line - it would only occur if the enum's definition has changed without also fixing this instance method. Throwing a RuntimeException suggests the caller made a mistake, when in fact the method (and the enum) itself is broken. Explicitly raising an AssertionError correctly indicates the invariants this method expects have been violated.

Guava has a helpful article which breaks down when to raise different types of exceptions. They write:

A conventional assertion is a check that should only fail if the class itself (that contains the check) is broken in some way. (In some cases this can extend to the package.) These can take various forms including postconditions, class invariants, and internal preconditions (on non-public methods).

An impossible-condition check is one that cannot possibly fail unless surrounding code is later modified, or our deepest assumptions about platform behavior are grossly violated. These should be unnecessary but are often forced because the compiler can't recognize that a statement is unreachable, or because we know something about the control flow that the compiler cannot deduce.

The page says an AssertionError is the recommended way to handle these cases. The comments in their Verify class also offers some useful insights about choosing exceptions. In cases where AssertionError seems too strong raising a VerifyException can be a good compromise.

As to the specific question of Error or RuntimeException, it doesn't really matter (both are unchecked and therefore will potentially travel up the call stack without being caught), but callers are more likely to attempt to recover from a RuntimeException. Crashing the application in a case like this is a feature, because otherwise we're continuing to run an application that is (at this point) demonstrably incorrect. It's certainly less likely that callers will catch and handle AssertionError (or Error or Throwable), but of course callers can do whatever they want.

Community
  • 1
  • 1
dimo414
  • 47,227
  • 18
  • 148
  • 244
  • So you would say that it is ok to throw an `AssertionError` actively in some cases? Or should that be only done by the compiler when the `assert` statement is used? – Mathias Bader Dec 25 '16 at 19:49
  • Certainly; an `AssertionError` indicates a program's invariants have been violated. – dimo414 Dec 25 '16 at 21:03
6

In my opinion, an AssertionError would be incorrect to use here.

From the docs, an AssertionError extends base class Error

An Error is a subclass of Throwable that indicates serious problems that a reasonable application should not try to catch.

An error should be fatal, whereas I would expect your program to handle this, and display the user a warning message about the unknown operation.

If anything here, I would expect an UnsupportedOperationException to be thrown, and handled elsewhere in the call stack.

Thrown to indicate that the requested operation is not supported.

Consider the case where, not in a calculator, but any code flow that uses ENUMs:

If a developer were to add a new value to an existing enum, I would not expect functions that make use of this existing enum to invoke an error, just because the new value is not supported.

Matt Clark
  • 27,671
  • 19
  • 68
  • 123
  • Sure it is debatable whether we should show a message or throw an error. But let's assume we want to throw an error: Which one would be the right one? Can it really be `AssertionError`? The docu also says _Thrown to indicate that an assertion has failed_. But here there was no assertion at all - so I have the feeling that this is against the documentation. Right? – Mathias Bader Dec 25 '16 at 19:09
  • See my edit. I would not necessarily want to use an assertion here at all. Assertions should be used to rise an error, when something known to be generally true, is not. – Matt Clark Dec 25 '16 at 19:18
  • It's not wrong to use assertionerror in this case as it asserts that this case will never be reached. – Gurwinder Singh Dec 25 '16 at 19:18
  • This is getting into personal code styling now - but in my opinion, an `UnsupportedOperationException` would be more ideal here. – Matt Clark Dec 25 '16 at 19:21
  • @MattClark: I also feel that `UnsupportedOperationException` would fit quite nicely here. Though I beg to differ that it is a question of personal code style. I think it should be clear whether we are allowed to actively throw an `AssertionError`or not. It can be either not allowed, it can be allowed but should not be done, or it can be allowed and could be used. That is the question I'm trying to evaluate. If we clarify that one, in the last case, it would then be a question of personal code style whether you actually use it or not. In the first case, it would actually be wrong. – Mathias Bader Dec 25 '16 at 19:25
  • 5
    I disagree; because the enum has a finite set of arguments it should be impossible to reach the `throw` line - it would only occur if the enum's definition has changed. Throwing an `IAE` or `UOE` suggests the caller made a mistake, when in fact the method is broken. An `AssertionError` correctly indicates the invariants this method expects still hold. – dimo414 Dec 25 '16 at 19:31
  • 3
    I'm with dimo. If we hit that `AssertionError`, it doesn't mean the caller did something they shouldn't have; it means we're in a case that should have been impossible. That'd normally be something we `assert` can't happen, but an `assert` isn't enough to satisfy the compiler's reachability analysis, so we `throw` an `AssertionError` outright. – user2357112 Dec 25 '16 at 21:14
  • @dimo414 I agee with you in the sense that an enum, at this point in time, has a defined set of arguments. Consider the case where a developer adds a new value to an existing enum. I would not expect _existing_ code to now throw an error, because of a new value here, it would simple be unsupported. – Matt Clark Dec 26 '16 at 04:39
  • 3
    Notice that the example is a method on the enum itself. I agree if the enum was an argument `IAE` *might* be reasonable (depending on the relationship between the enum and the method), but as part of the enum itself the method is simply miss-defined if a new enum value is added without fixing the method. – dimo414 Dec 26 '16 at 05:26
5

Regarding errors, the Java Tutorial states:

The second kind of exception is the error. These are exceptional conditions that are external to the application, and that the application usually cannot anticipate or recover from.

Also, the Programming With Assertions guide states:

Do not use assertions for argument checking in public methods.

So I think Exception are the right way to check for this kind of cases.

I recommend using a new UnsupportedOperationException("Operator " + name() + " is not supported."); since it better describe the problem in my opinion (i.e. a developer added an enum value but forgot to implement the required case).

However I think that this sample case should use an AbstractEnum design pattern instead of a switch:

PLUS {
    double apply(double x, double y) {
        return x + y;
    }
},
MINUS {
    double apply(double x, double y) {
        return x - y;
    }
},
TIMES {
    double apply(double x, double y) {
        return x * y;
    }
},
DIVIDE {
    double apply(double x, double y) {
        return x / y;
    }
};

abstract double apply(double x, double y);

It is less error prone since this code will not compile until every case implements apply.

Raphaël
  • 3,646
  • 27
  • 28
  • 1
    "*Do not use assertions for argument checking in public methods.*" Bloch's example is not checking a public method's argument - it's asserting that the *class itself* is correctly defined. I'd suggest reading the cited section of Effective Java; he goes on to explore the the pattern you describe, and explains why using an enum is sometimes preferable. – dimo414 Dec 25 '16 at 22:35
3

I would prefer

    double apply(double x, double y) {
    switch(this) {
        case PLUS:   return x + y;
        case MINUS:  return x - y;
        case TIMES:  return x * y;
        default: assert this==DIVIDE: return x / y;
    }
}
  1. We should not be throwing AssertionError because it should be reserved for actual assertions.
  2. Other than assertions and some catch blocks there should not be a single bit of code that can not practicably reached.

But I would most prefer https://stackoverflow.com/a/41324246/348975

Community
  • 1
  • 1
emory
  • 10,725
  • 2
  • 30
  • 58
1

I think both AssertionError or IllegalAE are not very good here. Assertion Error is not good as indicated in Matt's answer. And the arguments are not wrong here, those are just passed to a method on wrong this operation. So IAE may not be good as well. Of course this is an Opinion based question and answer as well.

Also, I am not sure enabling assertion is mandatory for throwing AssertionError or an assertionError means assertions were enabled.

Atul
  • 2,673
  • 2
  • 28
  • 34
0

As I understand, your method is a method of enum object. In most cases when somebody adds new enum value, he should modify "apply" method as well. You should throw UnsupportedOperationException in this case.

Ivan Dubynets
  • 342
  • 1
  • 3
  • 12