Tuesday, November 15, 2011

Switching on Enums: A Style Observation

JetBrains dotPeek Logo

I was digging through the framework today looking for another good dotPeek of the Week topic. I was perusing the Lazy<T> class and found an interesting snippet.

This post isn't quite like my previous dotPeek of the Weeks insofar as this is more of an example of what not to do. This is certainly merely my opinion, but one rule I try to follow when writing code is that more expressive is almost always better than less expressive. When I was looking at the Lazy class, I found a great example of this.

Here's the code (abridged for clarity … and also because the threading in this class will make for better discussion later):

private T LazyInitValue()
{
  switch (this.Mode)
  {
    case LazyThreadSafetyMode.None:
      // set the value
      break;

    case LazyThreadSafetyMode.PublicationOnly:
      // CompareExchange the value
      break;

    default:
      // lock and set values
      break;
  }
}

Is there anything you notice about this code? Perhaps any unanswered questions as you read it and try to figure out what it does? Specifically, what exactly constitutes the default case?

As I read through this code, learning about some of the interesting thread safety techniques, I found myself pondering, "why would locking be the default behavior? In fact, what is the default case? Do the default values have something in common?"

I looked at the enum LazyThreadSafetyMode and found this:

public enum LazyThreadSafetyMode
{
  None,
  PublicationOnly,
  ExecutionAndPublication,
}

That's when I decided that, in most cases, when you're switching on a reasonably small set of values, it's best to express these values explicitly so that the people (including you) who have to maintain the code can better understand why the default case is the default case … even if there are no comments.

For example, the following code is functionally equivalent:

private T LazyInitValue()
{
  switch (this.Mode)
  {
    case LazyThreadSafetyMode.None:
      // set the value
      break;

    case LazyThreadSafetyMode.PublicationOnly:
      // CompareExchange the value
      break;

    case LazyThreadSafetyMode.ExecutionAndPublication:
      // lock and set values
      break;
  }
}

Personally, I find the latter example much more expressive. It's obvious to me what the three cases are and I don't have to wonder what possible values can become the default case. In fact, I may even go so far as having the default case throw an exception in this class. I'd do this so that, for whatever reason, if someone were to change the LazyThreadSafetyMode enum and not implement that case for the new values in Lazy<T>.LazyInitValue(), they'd get an exception in testing instead of incorrectly using the default functionality.

No comments:

Post a Comment