CodeSOD: Exceptionally Backwards

"Generic exception handlers" sound like an oxymoron, but are a weirdly common pattern in web development. ASP .NET, for example, has a global.asax file, which can contain an Application_Error method. In practice, this is meant to handle all the otherwise unhandleable errors- each individual endpoint should still do exception handling for all the errors it can, but the errors that are impractical to handle locally, like the database being inaccessible, bubble up to your generic handler.

Of course, it's important that the generic handler actually handle the exception. Which brings us to George's submission, an ASP .Net error handler.

 protected void Application_Error(object sender, EventArgs e) { Exception ex = Server.GetLastError().GetBaseException(); SysGenericException myExp; string msg = string.Empty; switch (ex.GetType().ToString()) { case "FrameWork.Exceptions.SysException": myExp = (SysException)ex; msg = myExp.Message; break; case "FrameWork.Exceptions.SysDatabaseException": myExp = (SysDatabaseException)ex; msg = myExp.Message; break; case "FrameWork.Exceptions.SysNotifyException": myExp = (SysNotifyException)ex; msg = myExp.Message; break; case "FrameWork.Exceptions.SysInvalidPrivilegeException": myExp = (SysInvalidPrivilegeException)ex; msg = myExp.Message; break; default: ExceptionPolicy.HandleException(ex, MDBExceptionPolicy.Global); msg = "An Unexpected Application Error occurred"; break; } //Session["LastErrorMsg"] = msg; //Session["BackURL"] = Request.Url; Server.Transfer(PageURL.GenericError, false); }

We start by looking at the last error and tracking down its BaseException- that is to say, if exception A happened, and the handler code threw exception B, we want to get a reference to exception A- the root cause. On its surface, this isn't a bad idea, but as we'll see, in this instance, it's hiding a horrible choice.

We create a SysGenericException variable to hold our exception, which implies that we've created our own tree of exceptions for our application. I don't love that approach, but it does at least give us the benefit of polymorphism so that we can easily identify which exceptions are our custom exception, and which aren't.

And then we switch on the string name of the type of the exception. This is bad enough, but even worse: we do the same thing in every branch. There's no point in switching here. If we actually needed to cast to our internal exception type, we don't need a switch, we could use the is operator: if (ex is SysGenericException). But we don't need to actually do that cast- the only property we access is Message, which is built in on all Exception types.

Except we don't even need to do that, because the msg value is never used. It was used, once upon a time, and we stuffed the error into the Session object (likely so that we could print out the message for the user), but we're not doing that now.

We can also see that they're using an ExceptionPolicy object, likely a library designed to easily handle different error types in a declarative fashion, which means they likely should have just used that to handle all the exceptions, instead of trying to have a weird branching/type-checking thing here.

And we're not done. Because we have to go back to that GetBaseException call. Best practice is that, if you catch an exception and need to throw an application specific exception, you leverage the InnerException property: if the database is down, but you want to throw a SysDatabaseException, you should construct the SysDatabaseException so that it contains the database error. Then GetBaseException will return the database error, the root cause of your problem.

Which isn't what's happening here. They likely are catching built in exceptions, and constructing entirely new, parallel exception objects, but not using InnerExceptions to maintain all the error information. They're basically deleting all the root cause information so that they can use their own, internally defined, exception objects.

This is clearly a large, enterprise product. Likely the root cause of these problems- the inner exception if you will- is that you have developers who don't actually know the language or its tooling, reinventing wheels badly because they want the language to fit their intuitions instead of having to learn anything. Like a child who hasn't quite learned to dress themselves, they're wearing their underwear backwards and claiming that this is superior, because it addresses all the problems they have if they wear their underwear front-way round. The chafing is just something you have to live with.

[Advertisement] ProGet's got you covered with security and access controls on your NuGet feeds. Learn more.