CodeSOD: The Default Path

I've had the misfortune to inherit a VB .Net project which started life as a VB6 project, but changed halfway through. Such projects are at best confused, mixing idioms of VB6's not-quite object oriented programming with .NET's more modern OO paradigms, plus all the chaos that a mid-project lanugage change entails. Honestly, one of the worst choices Microsoft ever made (and they have made a lot of bad choices) was trying to pretend that VB6 could easily transition into VB .Net. It was a lie that too many managers fell for, and too many developers had to try and make true.

Maurice inherited one of these projects. Even worse, the project started in a municipal IT department then was handed of to a large consulting company. Said consulting company then subcontracted the work out to the lowest bidder, who also subcontracted out to an even lower bidder. Things spiraled out of control, and the resulting project had 5,188 GOTO statements in 1321 code files. None of the code used Option Explicit (which requires you to define variables before you use them), or Option Strict (which causes errors when you misuse implicit data-type conversions). In lieu of any error handling, it just pops up message boxes when things go wrong.

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

obj is defined as Object, but is in fact a TextBox. The function is called getDefaultPath, which is not what it seems to do. What does it do?

Well, it looks up the TAXBILLINGYEAR from a table called t_controltable. It runs this query by using a variable called goDB which thanks to hungarian notation, I know is a global object. I'm not going to get too upset about reusing a single database connection as a global object, but it's definitely hinting at other issues in the code.

We check only the first row from that query, which shows a great deal of optimism about how the data is actually stored in the table. While there are many ways to ensure that tables store data in sorted order, an ORDER BY clause would go a long way to making the query clear. Also, since we only need one row, a TOP N or some equivalent would be nice.

Then we use a global calendar object to do absolutely nothing in our if statement.

That leads us to the second query, which at least Categoryid is an integer and lEffTaxYear is a long, which makes this potential SQL injection not really an issue. We run that query, and then check the number of rows- a sane check which we didn't do for the last query- and then once again, only look at the first row.

I'm going to note that MSYSTYPEVALUE1 may or may not be a "default path", but I certainly have no idea what they're talking about and what data this function is actually getting here. The name of the function and the function of the function seem disconnected.

In any case, I especially like that it doesn't return a value, but directly mutates the text box, ensuring minimal reusability of the function. It could have returned a string, instead.

Speaking of returning strings, that gets us to our final bonus. It does return a string- a string of "True", using the "delightful" functionName = returnValue syntax. Presumably, this is meant to represent a success condition, but it only ever returns true, concealing any failures (or, more likely, just bubbling up an exception). The fact that the return value is a string hints at another code smell- loads of stringly typed data.

The "good" news is that what it took layers of subcontractors to destroy, Maurice's team is going to fix by June. Well, that's the schedule anyway.

[Advertisement] Utilize BuildMaster to release your software with confidence, at the pace your business demands. Download today!