Don’t Misuse “Else”

Or Else...

Over the years, I’ve taken over maintenance of quite a few systems.  I have made quite a few enhancements to one of the systems I took over maintenance for a year ago (call this System B).

There’s another system I’ve been enhancing for over 8 years (call this System A).  One thing I discovered while working on that system is that I prefer unexpected failure to throw exceptions as opposed to failing silently.

I recently had to add support for a new role for System B which turned out to be quite a bit of work because the original design and coding of this system didn’t plan for new roles.  I noticed what I consider to be a misuse of “else” that could easily allow a bug to sneak through when enhancing the code.

The code examples here are simplified to show just the point of this article.  Let’s assume for this example that we have an enumeration that defines the user roles and a user can only have one role.  Here are the currently defined roles:

public enum UserRoles
{
   Admin,
   SuperUser,
   SalesRep
}

If we use the following code to perform tasks based on the user role (which is similar to the pattern that was being used in the code I was modifying), it would work for now, but it is not the best solution:

if (userRole == UserRoles.Admin)
{
}
else if (userRole == UserRoles.SuperUser)
{
}
else
{
   // assumes that this is SalesRep
}

This code works for now, but imagine a new role “ServiceRep” gets added.  The “else” here will allow the new ServiceRep role to be treated just like a SalesRep user. Imagine this code is hidden away in tens of thousands of lines of code–the issue might not ever go noticed.

If you use the following code, when you add a new role, an exception will be thrown, allowing you to address the issue during development or testing.

if (userRole == UserRoles.Admin)
{
}
else if (userRole == UserRoles.SuperUser)
{
}
else if(userRole == UserRoles.SalesRep)
{
}
else
{
   throw new NotImplementedException("role not supported: " + userRole.ToString());
}

A similar pattern for switch statements works great too:

switch (userRole)
{
   case UserRoles.Admin:
      break;
   case UserRoles.SuperUser:
      break;
   case UserRoles.SalesRep:
      break;
   default:
      throw new NotImplementedException("role not supported: " + userRole.ToString());
}

avatar
About Sam (12 Articles)
IT professional and entrepreneur with over 30 years of computer experience. He is an independent contractor providing senior level technology consulting services with a focus on Microsoft ASP.NET solutions.

Leave a comment

Your email address will not be published.


*