Lance Hunt's C# Coding Standards - enum confusion

Question

My team has recently started using Lance Hunt's C# Coding Standards document as a starting point for consolidating our coding standards.

There is one item that we just don't understand the point of, can anyone here shed any light on it?

The item is number 77:

Always validate an enumeration variable or parameter value before consuming it. They may contain any value that the underlying Enum type (default int) supports.

Example:

public void Test(BookCategory cat)
{
if (Enum.IsDefined(typeof(BookCategory), cat))
{…}
}

Answer 1

The point is that you might hope that by having a parameter of type BookCategory, you'd always have a meaningful book category. That isn't the case. I could call:

BookCategory weirdCategory = (BookCategory) 123456;
Test(weirdCategory);

If an enum is meant to represent a well-known set of values, code shouldn't be expected to sensibly handle a value outside that well-known set. The test checks whether the argument is appropriate first.

I'd personally reverse the logic though:

public void Test(BookCategory cat)
{
    if (!Enum.IsDefined(typeof(BookCategory), cat))
    {
        throw new ArgumentOutOfRangeException("cat");
    }
}

In C# 3 this can easily be done with an extension method:

// Can't constrain T to be an enum, unfortunately. This will have to do :)
public static void ThrowIfNotDefined<T>(this T value, string name) where T : struct
{
    if (!Enum.IsDefined(typeof(T), value))
    {
        throw new ArgumentOutOfRangeException(name);
    }
}

Usage:

public void Test(BookCategory cat)
{
    cat.ThrowIfNotDefined("cat");
}

Answer 2

Enums are not checked:

enum Foo { A= 1, B = 2, C = 3}
Foo x = (Foo) 27; // works fine

Now you have a Foo that isn't defined. He is just saying "check your input". Note that Enum.IsDefined is relatively slow (reflection based). Personally, I tend to use a switch with a default that throws an exception:

switch(x) {
    case Foo.A: /* do something */ break;
    case Foo.B: /* do something */ break;
    case Foo.C: /* do something */ break;
    default: throw new ArgumentOutOfRangeException("x");
}

Answer 3

I think the comments above pretty much answered the question. Essentially, when I wrote this rule I was trying to convey a defensive coding practice of validating all inputs. Enums are a special case because many developers incorrectly assume that they are validated, when they are not. As a result, you will often see if statements or switch statements fail for an undefined enum value.

Just keep in mind that by default an enum is nothing more than a wrapper around an INT, and validate it just as if it were an int.

For a more detailed discussion of proper enum usage, you might check out blog posts by Brad Abrams and Krzysztof Cwalina or their excellent book "Framework Design Guidelines: Conventions, Idioms, and Patterns for Reusable .NET Libraries"

Answer 4

This is because it's totally legal to call that "Test" method like this:

Test((BookCategory)-999);

That'll cast -999 as a BookCategory, but of course the result (passed as the "cat" parameter to Test) isn't a valid value for the BookCategory enum.