CodeSOD: Exceptional Numbers

"When handling user input, make sure to handle exceptions," is good advice. But sometimes, that can lead you astray.

Bob is a junior programmer surrounded by veterans with 20+ years of experience. Normally, this means that when there's something unusual in the code, Bob writes it off as the senior devs vast experience. But not this time.

 Private Function IsANumber(ByVal theNumber As String) As Boolean Try If IsNumeric(theNumber) Then Return True Else Return False End If Catch ex As Exception err.Text = "There was an error." & ControlChars.CrLf & "Please try again later." err.CssClass = "cssLblError" Dim log As New logFile(Session("ConnectionString"), ex, "") End Try End Function

At first glance, this just looks like some pretty basic silliness. First, this really exists to be a wrapper around IsNumeric, a built-in function. Instead of the If/Else we could just Return IsNumeric(theNumber). Also, IsNumeric never throws an exception, making the exception handling entirely unnecessary.

But rereading it, there's more lurking here, via implication. This is an ASP.Net WebForms application. err is clearly a web control on that form, and here we write the error message directly to the control. And this suggests some broader problems.

First, that error message is terrible. It conveys no information at all. But worse, because we handle the exception right here and don't raise it up, we can assume execution will continue. Now again, in this case, we'd never hit the exception in the first place, but if we assume this is their exception-handling pattern, it strongly implies that no matter how many errors happen in handling user input, you only get one error message which tells you nothing about what actually went wrong. And if it's usually something like "try again later", that tells the user that their input was fine, and there's just something wrong in your application.

Finally, we create a class called logFile, which takes a ConnectionString as its input, which implies it's not logging to a file, but logging to a database, which just to make sure things get extra confusing.

So it's not that this code is some particular WTF horror. It's that this code is representative of these long-time software development veterans' efforts. This may just be a code smell, on its own, but there's a stench someplace in the codebase.

[Advertisement] BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!