Default Path – Daily WTF

I had the misfortune of inheriting a VB .Net project that started life as a VB6 project but changed halfway through. Such projects are at best confused, mixing the idioms of VB6’s not-quite-object-oriented programming with .NET’s more modern OO paradigms, plus all the chaos that switching languages ​​in the middle of a project entails. Honestly, one of the worst choices Microsoft ever made (and they made a lot of bad choices) was to pretend that VB6 could easily migrate to VB .Net. It was a lie that too many managers fell for, and too many developers had to try to make it true.

Maurice inherited one of those projects. Worse, the project started in the municipal IT department and was then handed over to a large consulting firm. The aforementioned consulting company then subcontracted the work to the lowest bidder, who also subcontracted with an even lower bidder. Things got out of hand, and the resulting project had 5,188 GOTO statements in 1321 code files. No code was used Option Explicit (which requires you to define variables before using them), or Option Strict (which causes errors when you abuse implicit data type conversions). Instead 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")
            
            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 Objectbut is actually a TextBox. The function is called getDefaultPath, which is not what it seems. What works do that?

Well, it looks worse TAXBILLINGYEAR from the so-called table t_controltable. Runs this query using a named variable goDB which, thanks to the Hungarian record, I know is a globular osubject. I’m not going to get too upset about reusing a single database connection as a global object, but it definitely points to other problems in the code.

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

Then we use the global calendar object to do absolutely nothing in ours if statement.

This leads us to another question, which at least Categoryid is an integer i lEffTaxYear is long, which makes this potential SQL injection not really a problem. We run that query, and then we check the number of rows – a reasonable check that we didn’t do for the last query – and then once again, we only look at the first row.

I’ll make a note of that MSYSTYPEVALUE1 it may or may not be “default path” but I sure have no idea what they are talking about and what data this function is actually getting here. Function name i function the functions seem disjointed.

Anyway, I especially like that it doesn’t return a value, but directly modifies the textbox, ensuring minimal function reuse. He could have returned the streak instead.

Speaking of comeback streaks, that brings us to the final bonus. It works return string – string “True”, using “delightful” functionName = returnValue syntax. Presumably, this should represent a success condition, but only that ever returns true, obscuring any errors (or, more likely, just throwing an exception). The fact that the return value is a string hints at another code smell – heaps of typed data.

The “good” news is that Maurice’s team will fix by June what was supposed to be destroyed by subcontractors. Well, that’s the schedule anyway.