Monday, August 29, 2011

Smelly code, smelly code. Why are they writing you?

There are certain times in your career as a developer when you come across code so horrible, it causes you to question your faith in humanity. Such was the case when my boss and I were given the MicroMain Web Request application to install on one of our servers. We couldn't get it work even though we followed the instructions meticulously. We even spoke to one of their support people. Unfortunately, he was not one of their headset hotties. The error message we were getting wasn't too helpful, either:

Format of the initialization string does not conform to specification starting at index 0

This is a generic .net message. The issue was that we couldn't connect to the database. However, our SQL Server hadn't recorded a login attempt from the application. We were thinking it was a configuration issue. The web.config file was valid xml. Nothing looked terribly strange. Thankfully, they also provided us with the source code. Probably an accident. Nevertheless, we went to the function that was causing the error and were horrified by what we saw.

Public Shared Function GetConnectionString() As String
   'Dim sOleDb As String
   'Dim cn As OleDbConnection
   'Dim cmd As OleDbCommand
   'Dim dr As OleDbDataReader
   Dim returnConnString As String
   Dim sMethod, sColumns, sTableName As String
   sColumns = "DataLink, Platform, Login, Password, Server, Database"
   sTableName = "tblzApp"
   sMethod = ""
   If ConfigurationManager.AppSettings.Count > 0 Then
       sMethod = ConfigurationManager.AppSettings.GetKey(0)
   End If
   Try
       Select Case sMethod
           Case "ConnectionString"
               returnConnString = ConfigurationManager.AppSettings(sMethod).ToString
           Case Else
               Return sMethod
       End Select
       Return returnConnString
   Catch ex As Exception
       ex.Source &= "<br /> DAL.GetConnectionString"
       Throw ex
   End Try
End Function

Just for clarification, this is a 23 line function that should be a 1 line function. Here is what the body should look like:

Return ConfigurationManager.ConnectionStrings("MicroMain")

The beauty of this 1 line function is that if it fails, you know exactly why it failed. It fails fast. Conversely, their code fails slowly. In other words, it failed but didn't tell us why.

In addition to that, I happened upon at least 9 different code smells within this single function.

Code Smells within GetConnectionString

  1. getting a key/value item by position instead of name (appSettings is a key/value store)

    This is the equivalent of being a high school principal and walking into a classroom to fetch Jonnie. Instead of walking in and asking for Jonnie, you walk in and grab the student closest to the door, whisper into his ear "Are you Jonnie?". If he says "no", you walk out the door and do not complete your objective.

  2. using AppSettings for the connection string instead of ConnectionStrings (non-idiomatic)

    Since 2005, .net has a config section dedicated to connection strings.

  3. declaring unused variables (ie copy and paste programming)

  4. declaring unnecessary temporary variables (eg returnConnString)

  5. unnecessarily calling ToString

    Getting an AppSetting value implicitly returns a string. Calling ToString simply gives you the opportunity to fail with the generic error message "object not set to instance of object" instead of letting the caller handle the missing value case.

  6. try/catch is totally unnecessary (since ToString should not be called)

    Furthermore, try/catch blocks should not be put into most methods. Unhandled exceptions should be allowed to pop up the stack to a global exception handler.

  7. mixing html into data access code

    If I have to explain why this is bad, you need to find another career.

  8. commented out code still hanging around (lazy coding or perhaps they don't use source control!!!)

  9. naming the connection string key "ConnectionString" instead of "MicroMainConnectionString"

    This assumes that the application is the only application deployed on the server.

The disaster continues

  • no global exception handler (ie yellow screen of death)
  • SQL Injection attacks

    Looking through other methods within their code, I found them not using parametrized queries. Instead, they were building SQL by string concatenation which leaves them wide open to SQL injection attacks.

Makes you think...

Looking at their website and taking into account the fact that they sold their software to Newell-Rubbermaid (a Fortune 500 company), I get the impression that they have better salespeople than developers. Their code was head-scratchingly bad. It makes you wonder about the state of code in other off-the-shelf applications. Is it all this bad, or was this an anomaly?

As Jeff Atwood would say "Enterprisey to the bone".