tags:

views:

3452

answers:

9

For people suggesting throwing an exception:
Throwing an exception doesn't give me a compile-time error, it gives me a runtime error. I know I can throw an exception, I'd rather die during compilation than during runtime.

First-off, I am using eclipse 3.4.

I have a data model that has a mode property that is an Enum.

enum Mode {on(...), off(...), standby(...); ...}

I am currently writing a view of this model and I have the code

...
switch(model.getMode()) {
case on:
   return getOnColor();
case off:
   return getOffColor();
case standby:
   return getStandbyColor();
}
...

I am getting an error "This method must return a result of type java.awt.Color" because I have no default case and no return xxx at the end of the function. I want a compilation error in the case where someone adds another type to the enum (e.g. shuttingdown) so I don't want to put a default case that throws an AssertionError, as this will compile with a modified Mode and not be seen as an error until runtime.

My question is this:
Why does EclipseBuilder (and javac) not recognize that this switch covers all possibilities (or does it cover them?) and stop warning me about needing a return type. Is there a way I can do what I want without adding methods to Mode?

Failing that, is there an option to warn/error on switch statements that don't cover all of the Enum's possible values?

Edit: Rob: It is a compile error. I just tried compiling it with javac and I get a "missing return statement" error targeting the last } of the method. Eclispe just places the error at the top of the method.

+1  A: 

Create a default case that throws an exception:

throw new RuntimeExeption("this code should never be hit unless someone updated the enum")

... and that pretty much describes why Eclipse is complaining: while your switch may cover all enum cases today, someone could add a case and not recompile tomorrow.

kdgregory
+1  A: 

Why does EclipseBuilder not recognize that this switch covers all possibilities (or does it cover them?) and stop warning me about needing a return type. Is there a way I can do what I want without adding methods to Mode?

It's not an issue in Eclipse, but rather the compiler, javac. All javac sees is that you don't have a return value in the case in which nothing is matched (the fact that you know you are matching all cases is irrelevant). You have to return something in the default case (or throw an exception).

Personally, I'd just throw some sort of exception.

mipadi
I know this is nitpicky, but Eclipse doesn't use javac. It has its own internal Java compiler.
Adam Crume
You'll notice that I specified EclipseBuilder as generating the error. Also, throwing an exception doesn't give me a compile-time error, it gives me a runtime error, which is what I though Enums were supposed to provide (compared to using ints like c-enums).
KitsuneYMG
Little known fact: the Eclipse JDT actually include their own compiler separte from javac. http://www.eclipse.org/jdt/core/index.php
bendin
+2  A: 

I'd say it's probably because model.GetMode() could return null.

McWafflestix
yup. enums are refence types.
bendin
I just checked. I had assumed Sun knew wtf they were doing with Enums, but apparently you can assign null to an enum type. Whose braindead idea was that. Thanks for pointing this out.
KitsuneYMG
That doesn't make sense. That would lead to a NullPointerException anyway. You can't define a "case null"
amarillion
+1  A: 

Since I can't just comment...

  1. Always, Always, Always have a default case. You'd be surprised how "frequently" it would be hit (Less in Java than C, but still).

  2. Having said that, what if I only want to handle only on/off in my case. Your semantic processing by javac would flag that as an issue.

Paul
For (2) you would have a default case covering enums you're not handling. I think the issue is that he's handling every enum already, so he shouldn't get a "no return value" error during compile.
Steve Armstrong
+1  A: 

Your problem is that you are trying to use the switch statement as an indicator that your enum is locked down.

The fact is that the 'switch' statement and the java compiler cannot recognize that you do not want to allow other options in your enum. The fact that you only want three options in your enum is completely separate from your design of the switch statement, which as noted by others should ALWAYS have a default statement. (In your case it should throw an exception, because it is an unhandled scenario.)

You should liberally sprinkle your enum with comments so that everyone knows not to touch it, and you should fix your switch statement to throw errors for unrecognized cases. That way you've covered all the bases.

EDIT

On the matter of throwing compiler error. That does not strictly make sense. You have an enum with three options, and a switch with three options. You want it to throw a compiler error if someone adds a value to the enum. Except that enums can be of any size, so it doesn't make sense to throw a compiler error if someone changes it. Furthermore, you are defining the size of your enum based on a switch statement which could be located in a completely different class.

The internal workings of Enum and Switch are completely separate and should remain uncoupled.

+2  A: 

I don't know why you get this error, but here is a suggestion, Why don't you define the color in the enum itself? Then you can't accidentally forget to define a new color.

For example:

import java.awt.Color;

public class Test {

    enum Mode 
    {
     on (Color.BLACK), 
     off (Color.RED),
     standby (Color.GREEN);

     private final Color color; 
     Mode (Color aColor) { color = aColor; }
     Color getColor() { return color; }
    }

    class Model
    {
     private Mode mode;
     public Mode getMode () { return mode; }
    }

    private Model model;

    public Color getColor()
    {
     return model.getMode().getColor();
    } 
}

btw, for comparison here is the original case, with compiler error.

import java.awt.Color;
public class Test {

    enum Mode {on, off, standby;}

    class Model
    {
     private Mode mode;
     public Mode getMode () { return mode; }
    }

    private Model model;

    public Color getColor()
    {
     switch(model.getMode()) {
     case on:
        return Color.BLACK;
     case off:
        return Color.RED;
     case standby:
        return Color.GREEN;
     }
    } 
}
amarillion
+13  A: 

You have to enable in Eclipse (window -> preferences) settings "Enum type constant not covered in switch" with Error level.

Throw an exception at the end of the method, but don't use default case.

public String method(Foo foo)
  switch(foo) {
  case x: return "x";
  case y: return "y";
  }

  throw new IllegalArgumentException();
}

Now if someone adds new case later, Eclipse will make him know he's missing a case. So don't ever use default unless you have really good reasons to do so.

egaga
I wanted to let you know the reason I accepted the other answer instead of yours. Yours work great ... as long as everyone uses Eclispe with that options set. The answer I selected works with all java compilers. If I could rate you up again I would.
KitsuneYMG
Thanks for commenting, I just hoped you would notice this after my editing. I forgot to include the most important part in the first draft. I'm glad I could help.
egaga
+17  A: 

You could always use the Enum with Visitor pattern:

enum Mode
{
  on
  {
      public <E> E accept( ModeVisitor<E> visitor )
      {
         return visitor.visitOn();
      }
  },
  off
  {
      public <E> E accept( ModeVisitor<E> visitor )
      {
         return visitor.visitOff();
      }
  },
  standby
  {
      public <E> E accept( ModeVisitor<E> visitor )
      {
         return visitor.visitStandby();
      }
  }

  public abstract <E> E accept( ModeVisitor<E> visitor );

  public interface ModeVisitor<E>
  {
      E visitOn();
      E visitOff();
      E visitStandby();
  }
}

Then you would implement something like the following:

public final class ModeColorVisitor implements ModeVisitor<Color>
{
    public Color visitOn()
    {
       return getOnColor();
    }

    public Color visitOff()
    {
       return getOffColor();
    }

    public Color visitStandby()
    {
       return getStandbyColor();
    }

}

You'd use it as follows:

return model.getMode().accept( new ModeColorVisitor() );

This is a lot more verbose but you'd immediately get a compile error if a new enum was declared.

mtpettyp
This was selected because it does what I need/want. It allows arbitrary logic based on the enum state, w/o require that the enum track things that make no sense for it to track.
KitsuneYMG
A: 

A nice way for this would be to add the default case to return some error value or throw exception and to use automated tests with jUnit, for example:

@Test
public void testEnum() {
  for(Mode m : Mode.values() {
    m.foobar(); // The switch is separated to a method
    // If you want to check the return value, do it (or if there's an exception in the 
    // default part, that's enough)
  }
}

When you got automated tests, this will take care of that foobar is defined for all enumerations.

Touko