CodeSOD: Subbing for the Subcontractors

Back in the mid-2000s, Maurice got less than tempting offer. A large US city had hired a major contracting firm, that contracting firm had subcontracted out the job, and those subcontractors let the project spiral completely out of control. The customer and the primary contracting firm wanted to hire new subcontractors to try and save the project.

As this was the mid-2000s, the project had started its life as a VB6 project. Then someone realized this was a terrible idea, and decided to make it a VB.Net project, without actually changing any of the already written code, though. That leads to code like this:

Private Function getDefaultPath(ByRef obj As Object, ByRef Categoryid As Integer) As String Dim sQRY As String Dim dtSysType As New DataTable Dim iMPTaxYear As Integer Dim lEffTaxYear As Long Dim dtControl As New DataTable Const sSDATNew As String = "NC" getDefaultPath = False sQRY = "select TAXBILLINGYEAR from t_controltable" dtControl = goDB.GetDataTable("Control", sQRY) iMPTaxYear = dtControl.Rows(0).Item("TAXBILLINGYEAR") 'iMPTaxYear = CShort(cmbTaxYear.Text) If goCalendar.effTaxYearByTaxYear(iMPTaxYear, lEffTaxYear) Then End If sQRY = " " sQRY = "select * from T_SysType where MTYPECODE = '" & sSDATNew & "'" & _ " and msystypecategoryid = " & Categoryid & " and meffstatus = 'A' and " & _ lEffTaxYear & " between mbegTaxYear and mendTaxYear" dtSysType = goDB.GetDataTable("SysType", sQRY) If dtSysType.Rows.Count > 0 Then obj.Text = dtSysType.Rows(0).Item("MSYSTYPEVALUE1") Else obj.Text = "" End If getDefaultPath = True End Function

Indentation as the original.

This function was the culmination of four years of effort on the part of the original subcontractor. The indentation is designed to make this difficult to read- wait, no. That would imply that the indentation was designed. This random collection of spaces makes the code hard to read, so let's get some big picture stuff.

It's called getDefaultpath and returns a String. That seems reasonable, so let's skip down to the return statement, which of course is done in its usual VB6 idiom, where we set the function name equal to the result: getDefaultPath = True Oh... so it doesn't return the path. It returns "True". As a string.

Tracing through, we first query t_controltable to populate iMPTaxYear. Once we have that, we can do this delightful check:

 If goCalendar.effTaxYearByTaxYear(iMPTaxYear, lEffTaxYear) Then End If

Then we do some string concatenation to build a new query, and for a change, this is an example that doesn't really open up any SQL injection attacks. All the fields are either numerics or hard-coded constants. It's still awful, but at least it's not a gaping security hole.

That gets us a set of rows from the SysType table, which we can then consume:

 If dtSysType.Rows.Count > 0 Then obj.Text = dtSysType.Rows(0).Item("MSYSTYPEVALUE1") Else obj.Text = "" End If

This is our "return" line. You wouldn't know it from the function signature, but obj as Object is actually a textbox. So this function runs a pair of queries against the database to populate a UI element directly with the result.

And this function is just one small example. Maurice adds:

There are 5,188 GOTO statements in 1321 code files. Error handling consists almost entirely of a messagebox, and nowhere did they use Option Strict or Option Explicit.

There's so much horror contained in those two sentences, right there. For those that don't know VisualBasic, Option Strict and Option Explicit are usually enabled by default. Strict forces you to respect types- it won't do any late binding on types, it won't allow narrowing conversions between types. It would prohibit calling obj.Text =... like we see in the example above. Explicit requires you to declare variables before using them.

Now, if you're writing clean code in the first place, Option Strict and Option Explicit aren't truly required- a language like Python, for example, is neither strict no explicit. But a code base like this, without those flags? Madness.

Maurice finishes:

This is but one example from the system. Luckily for the city, what took the subcontractors 4 years to destroy only took us a few months to whip into shape.

[Advertisement] Otter - Provision your servers automatically without ever needing to log-in to a command prompt. Get started today!