Nov 30, 2010

Needless If Statements

I was looking through some code today and came across this:

context.HttpContext.Response.Clear();

if (ImageFormat.Equals(ImageFormat.Bmp))
context.HttpContext.Response.ContentType = "image/bmp";
if (ImageFormat.Equals(ImageFormat.Gif))
context.HttpContext.Response.ContentType = "image/gif";
if (ImageFormat.Equals(ImageFormat.Icon))
context.HttpContext.Response.ContentType = "image/vnd.microsoft.icon";
if (ImageFormat.Equals(ImageFormat.Jpeg))
context.HttpContext.Response.ContentType = "image/jpeg";
if (ImageFormat.Equals(ImageFormat.Png))
context.HttpContext.Response.ContentType = "image/png";
if (ImageFormat.Equals(ImageFormat.Tiff))
context.HttpContext.Response.ContentType = "image/tiff";
if (ImageFormat.Equals(ImageFormat.Wmf))
context.HttpContext.Response.ContentType = "image/wmf";

You’ve all seen this sort of thing before, right? And no doubt it’s cousin, the switch statement, is also very familiar to you.

The problem with that this sort of thing is that those if statements just aren’t required.  For amusement and/or education, go have a look at the Anti-If campaign to get a more detailed view as to why If statements are undesirable.

so when you see this sort of thing, take the few minutes you need to refactor your code to something like the following:

var contentResponses = new Dictionary<ImageFormat, string>()
{
{ImageFormat.Bmp, "image/bmp"},
{ImageFormat.Gif, "image/gif"},
{ImageFormat.Icon, "image/vnd.microsoft.icon"},
{ImageFormat.Jpeg, "image/jpeg"},
{ImageFormat.Png, "image/png"},
{ImageFormat.Tiff, "image/tiff"},
{ImageFormat.Wmf, "image/wmf"}
};

context.HttpContext.Response.Clear();
context.HttpContext.Response.ContentType = contentResponses[ImageFormat];

You’ll have fewer lines of code and the person who next looks at your code will appreciate it.

2 comments:

  1. The only thing to watch out for is the potential for a KeyNotFoundException, perhaps when someone adds a value to an enum but forgets to update the dictionary to match.

    ReplyDelete
  2. True. Though you have the same problem either way. One method fails fast whilst the other fails leaving the application in an unknown state.

    ReplyDelete