The danger of over simplification: Enum.IsDefined()
I just
stumbled across this guideline in the Design Guidelines document and I thought
it needed more explanation..
Do
argument validation. Do not assume
enum
arguments will be in the defined range. It is legal to cast any integer value
into an enum even if the value is not defined in the enum.
public void
PickColor(Color color) {
switch
(color) {
case
Red:
...
break;
case
Blue:
...
break;
case
Green:
...
break;
//repeat
for all known values of Color
default:
throw
new ArgumentOutOfRangeException();
break;
}
//do
work
}
Note: do not use Enum.IsDefined() for these checks as it
is based on the runtime type of the enum which can change out of sync with this
code.
There are really two problems with
Enum.IsDefined(). First it loads
reflection and a bunch of cold type metadata making it a deceptively expensive
call.
Secondly, as the note alludes to
there is a versioning issue here.
Consider an alternate way to write the method defined
above:
public void PickColor(Color color)
{
if (!Enum.IsDefined
(typeof(Color), color) {
throw new
InvalidEnumArgumentException("color", (int) color,
typeof(Color));
}
//Security issue: never pass a
negative color value
NativeMethods.SetImageColor
(color, byte[] image);
}
Callsite:
Foo.PickColor
((Color) -1); //throws
InvalidEnumArgumentException()
This
looks pretty good, even if you know this (mythical) native API has a buffer
overrun if you pass a color value that is negative. You know this because you know the
enum only defined postive values and you are sure that any value passed was one
of the ones defined in the enum right?
Well, only ½ right. You
don’t know what values are defined in the enum. Checking at the moment you write this
code is not good enough because IsDefined takes the value of the enum at
runtime. So if later someone added
a new value (say Ultraviolate = -1) to the enum IsDefined will start allowing
the value -1 one through. This is
true whether the enum is defined in the same assembly as the method or another
assembly.
public
Color {
Red = 1,
Green = 2,
Blue = 3,
Ultraviolate = -1, //new value
added this version
}
Now, that same callsite no longer
throws.
Callsite:
Foo.PickColor
((Color) -1); //causes a buffer overrun in
NativeMethods.SetImageColor()
The moral of the story is also two
fold. First be very careful when
you use Enum.IsDefined in your code.
The Second is when you design an API to simplify a situation, but sure
the fix isn’t worse than the current problem.
