What is the preferred method for handling unexpected enum values?

Question

Suppose we have a method that accepts a value of an enumeration. After this method checks that the value is valid, it switches over the possible values. So the question is, what is the preferred method of handling unexpected values after the value range has been validated?

For example:

enum Mood { Happy, Sad }

public void PrintMood(Mood mood)
{
    if (!Enum.IsDefined(typeof(Mood), mood))
    {
        throw new ArgumentOutOfRangeException("mood");
    }

    switch (mood)
    {
        case Happy: Console.WriteLine("I am happy"); break;
        case Sad:   Console.WriteLine("I am sad"); break;
        default: // what should we do here?
    }

What is the preferred method of handling the default case?

• Leave a comment like // can never happen
• Debug.Fail() (or Debug.Assert(false))
• throw new NotImplementedException() (or any other exception)
• Some other way I haven't thought of

Answer 1

I guess most of the above answers are valid, but I'm not sure any are correct.

The correct answer is, you very rarely switch in an OO language, it indicates you are doing your OO wrong. In this case, it's a perfect indication that your Enum class has problems.

You should just be calling Console.WriteLine(mood.moodMessage()), and defining moodMessage for each of the states.

If a new state is added--All Your Code Should Adapt Automatically, nothing should fail, throw an exception or need changes.

Edit: response to comment.

In your example, to be "Good OO" the functionality of the file mode would be controlled by the FileMode object. It could contain a delegate object with "open, read, write..." operations that are different for each FileMode, so File.open("name", FileMode.Create) could be implemented as (sorry about the lack of familiarity with the API):

open(String name, FileMode mode) {
    // May throw an exception if, for instance, mode is Open and file doesn't exist
    // May also create the file depending on Mode
    FileHandle fh = mode.getHandle(name);
    ... code to actually open fh here...
    // Let Truncate and append do their special handling
    mode.setPosition(fh);
}

This is much neater than trying to do it with switches... (by the way, the methods would be both package-private and possibly delegated to "Mode" classes)

When OO is done well, every single method looks like a few lines of really understandable, simple code--TOO simple. You always get the feeling that there is some big messy "Cheese Nucleus" holding together all the little nacho objects, but you can't ever find it--it's nachos all the way down...

Answer 2

I prefer to throw new NotImplementedException("Unhandled Mood: " + mood). The point is that the enumeration may change in the future, and this method may not be updated accordingly. Throwing an exception seems to be the safest method.

I don't like the Debug.Fail() method, because the method may be part of a library, and the new values might not be tested in debug mode. Other applications using that library can face weird runtime behaviour in that case, while in the case of throwing an exception the error will be known immediately.

Note: NotImplementedException exists in commons.lang.

Answer 3

In Java, the standard way is to throw an AssertionError, for two reasons:

1. This ensures that even if asserts are disabled, an error is thrown.
2. You're asserting that there are no other enum values, so AssertionError documents your assumptions better than NotImplementedException (which Java doesn't have anyway).

Answer 4

My opinion is that since it is a programmer error you should either assert on it or throw a RuntimException (Java, or whatever the equivalent is for other languages). I have my own UnhandledEnumException that extends from RuntimeException that I use for this.

Answer 5

The correct program response would be to die in a manner that will allow the developer to easily spot the problem. mmyers and JaredPar both gave good ways to do that.

Why die? That seems so extreme!

The reason being that if you're not handling an enum value properly and just fall through, you're putting your program into an unexpected state. Once you're in an unexpected state, who knows what's going on. This can lead to bad data, errors that are harder to track down, or even security vulnerabilities.

Also, if the program dies, there's a much greater chance that you're going to catch it in QA and thus it doesn't even go out the door.

Answer 6

For pretty much every switch statement in my code base, I have the following default case

switch( value ) { 
...
default:
  Contract.InvalidEnumValue(value);
}

The method will throw an exception detailing the value of the enum at the point an error was detected.

public static void InvalidEnumValue<T>(T value) where T: struct
{
    ThrowIfFalse(typeof(T).IsEnum, "Expected an enum type");
    Violation("Invalid Enum value of Type {0} : {1}", new object[] { typeof(T).Name, value });
}

Answer 7

For C#, something worth knowing is that Enum.IsDefined() is dangerous. You can't rely on it like you are. Getting something not of the expected values is a good case for throwing an exception and dying loudly.

In Java, it's different because enums are classes not integers so you really can't get unexpected values (unless the enum is updated and your switch statement isn't), which is one big reason why I much prefer Java enums. You also have to cater for null values. But getting a non-null case you don't recognize is a good case for throwing an exception too.

Answer 8

You could have a trace for the default calling out the value of the passed enum. Throwing exceptions is OK but in a large application there will be several places where your code does not care about other values of the enum.
So, unless you are sure that the code intends to handle all possible values of the enum, you'll have to go back later and remove the exception.

Answer 9

This is one of those questions that proves why test driven development is so important.

In this case I'd go for a NotSupportedException because literally the value was unhandled and therefore not supported. A NotImplementedException gives more the idea of: "This is not finished yet" ;)

The calling code should be able to handle a situation like this and unit tests can be created to easily test these kind of situations.

Answer 10

Call it an opinion or a preference, but the idea behind the enum is that it represents the full list of possible values. If an "unexpected value" is being passed around in code, then the enum (or purpose behind the enum) is not up to date. My personal preference is that every enum carry a default assignment of Undefined. Given that the enum is a defined list, it should never be out-of-date with your consuming code.

As far as what to do if your function is getting either an unexpected value or Undefined in my case, a generic answer doesn't seem possible. For me, it depends on the context of the reason for evaluating the enum value: is it a situation where code execution should halt, or can a default value be used?

Answer 11

It is the responsibility of the calling function to provide valid input and implicitely anything not in the enum is invalid (Pragmatic programmer seems to imply this). That said, this implies that any time you change your enum, you must change ALL code that accepts it as input (and SOME code that yields it as output). But that is probably true anyways. If you have an enum that changes often, you probably should be using something other than an enum, considering that enums are normally compile-time entities.

Answer 12

I usually try to define undefined value (0):

enum Mood { Undefined = 0, Happy, Sad }

That way I can always say:

switch (mood)
{
    case Happy: Console.WriteLine("I am happy"); break;
    case Sad:   Console.WriteLine("I am sad"); break;
    case Undefined: // handle undefined case
    default: // at this point it is obvious that there is an unknown problem
             // -> throw -> die :-)
}

This is at least how I usually do this.