CodeSOD: Exceptionally General

Andres noticed this pattern showing up in his company's code base, and at first didn't think much of it:

try{/*code here*/}catch (Exception ex){ ExceptionManager.HandleException(ex); throw ex;}

It's not uncommon to have some sort of exception handling framework, maybe some standard logging, or the like. And even if you're using that, it may still make sense to re-throw the exception so another layer of the application can also handle it. But there was just something about it that got Andres's attention.

So Andres did what any curious programmer would do: checked the implementation of HandleException.

public static Exception HandleException(Exception ex){ if (ex is ArgumentException) return new InvalidOperationException("(ExceptionManager) Ocurrio un error en el argumento."); if (ex is ArgumentOutOfRangeException) return new InvalidOperationException("(ExceptionManager) An error ocurred because of an out of range value."); if (ex is ArgumentNullException) return new InvalidOperationException("(ExceptionManager) On error ocurred tried to access a null value."); if (ex is InvalidOperationException) return new InvalidOperationException("(ExceptionManager) On error ocurred performing an invalid operation."); if (ex is SmtpException) return new InvalidOperationException("(ExceptionManager)An error ocurred trying to send an email."); if (ex is SqlException) return new InvalidOperationException("(ExceptionManager) An error ocurred accessing data."); if (ex is IOException) return new InvalidOperationException("(ExceptionManager) An error ocurred accesing files."); return new InvalidOperationException("(ExceptionManager) An error ocurred while trying to perform the application.");}

So, what this code is trying to do is bad: it wants to destroy all the exception information and convert actual meaningful errors into generic InvalidOperationExceptions. If this code did what it intended to do, it'd be destroying the backtrace, concealing the origin of the error, and make the application significantly harder to debug.

Fortunately, this code actually doesn't do anything. It constructs the new objects, and returns them, but that return value isn't consumed, so it just vanishes into the ether. Then our actual exception handler rethrows the original exception.

The old saying is "no harm, no foul", and while this doesn't do any harm, it's definitely quite foul.

[Advertisement] Continuously monitor your servers for configuration changes, and report when there's configuration drift. Get started with Otter today!