0

I just struggled over this code:

int convert(ChangeType changeType) {
  switch (changeType) {
    case CREATE:
      return 1;
    case MODIFY:
      return 2;
    case DELETE:
      return 3;
    default:
      throw new InternalError("changeType is null");
  }
}

And the corresponding enum:

public enum ChangeType {
  CREATE, MODIFY, DELETE;
}

I wondered: The default clause is obviously wrong, because it is meant to catch null, but null will call an NPE on line 2 of the listing.

But while experimenting, I found that removing the default case ends up in the compile error "missing return statement", but without adding more values to the enum this switch is "complete" and there is no such uncovered code path.

Q: So, is it really expected (as a user of this enum), that I add a default case which is actually a dead (and untestable) code path unless someone changes the enum?

Instead, I would even expect that code analyzers like Sonar warn if there is a complete switch and a default case.

PS: The code is only an example. I know there is Enum.ordinal() which replaces most of the above code

Daniel Alder
  • 5,031
  • 2
  • 45
  • 55
  • 2
    `The default clause is obviously wrong` : no. `because it is meant to catch null`: **no** – specializt Mar 19 '19 at 11:24
  • In case of default just return 0 or negative value. – Sudhir Ojha Mar 19 '19 at 11:25
  • 2
    for enum strictly iirc, java-12 will fix this, as no `default` would be needed – Eugene Mar 19 '19 at 11:26
  • @specializt Read my text: if changeType is null, I get an NPE and the default case doesn't come to action – Daniel Alder Mar 19 '19 at 11:29
  • @Eugene I think that's incorrect. I believe it will only apply to switch expressions, not statements. "in the case of an enum switch **expression** that covers all known cases ..., a default clause can be inserted by the compiler" ([JEP 325](https://openjdk.java.net/jeps/325)) In this case, the OP could convert the statement to an expression quite easily though. – Michael Mar 19 '19 at 11:37
  • @Michael good point! thank u – Eugene Mar 19 '19 at 11:38
  • 1
    @Eugene No worries. I was going to be pedantic about you saying "java-12 **will** fix this" (because it's released today) but it turns out you were right (later today, I suppose), so I had to be pedantic about something else – Michael Mar 19 '19 at 11:42
  • @Michael is there an award for pedantic of the day? :) – Eugene Mar 19 '19 at 11:45
  • @DanielAlder of course you do - everything else would be a very weird language or VM bug. Default cases are **not** supposed to be entered for `null` values - ever ... and thats what we observe, its why NPEs even exist -- to halt or to indicate serious programming mistakes which can only be worked around with `try` and `catch` – specializt Mar 19 '19 at 12:02
  • @Lino : no, he didnt. – specializt Mar 19 '19 at 12:04

1 Answers1

1

The default case with an exception serves as helper in case you ever introduce new ChangeType value. Say you added fourth type UPSERT. This switch statement might be in multiple places in your code base which are only designed to handle CREATE, MODIFY, DELETE and don't expect new UPSERT value. The exception is a fail-fast approach that will immediately tell you that something went not as expected.

Karol Dowbecki
  • 43,645
  • 9
  • 78
  • 111
  • 1
    in java-12, this `default` would not be needed in case of enums – Eugene Mar 19 '19 at 11:27
  • @Eugene still worth adding this superficial `default` with a `throw` if the code is to fail-fast. It just doesn't really make sense for a `null` which should be checked with precondition. – Karol Dowbecki Mar 19 '19 at 11:28
  • I can't understand why, I never liked this, never. – Eugene Mar 19 '19 at 11:28
  • 1
    @Eugene : it is **never** _needed_. But it is usually considered bad practise to leave it out because default cases **will** reveal programming mistakes later on – specializt Mar 19 '19 at 12:18
  • 1
    @Eugene As clarified in [this comment](https://stackoverflow.com/questions/55239849/why-does-my-java-compiler-force-a-default-case-in-a-complete-switch-statement#comment97212795_55239849), Java 12 will eliminate the need for `default` with switch expressions only and in that case, there’s truly no need to add a `default`, as the compiler will precisely generate a fail-fast, throwing `default` branch. That would match the OP’s use case, when converting the code, as there *must* be a `return` or `throw` at the method end. – Holger Mar 19 '19 at 14:58
  • @KarolDowbecki your answer is correct regarding why a `default` is needed, however, the error message of the OP’s code example, `"changeType is null"` is indicating that the author had a different, wrong mindset. – Holger Mar 19 '19 at 15:04