When you can't code, just add more counters



  • Up until a couple years ago, a substantial amount of reporting in one division of a major corporation was done manually.  Yes, the numbers were generated by automated systems, but the multiple pieces of proprietary software didn't cross-communicate, so reports were printed off, and manually error checked using the time-honored stare and compare algorithm.  Although a WTF on its own, that's not really the fun part.  One supervisor finally put forth the effort to fix the problem, creating a series of ASP classic webapps hosted on the company intranet to query the SQL servers that actually hosted the data, organize it, and do all the comparisons and error tests automatically.  Sure, it was Q4 2007, but the information age had begun!  The problem, of course, is that the original "developer" was nothing of the sort.  He didn't actually know how to code in vbscript.  Or, frankly, at all.  Much of his work consisted of hitting Google, finding similar-appearing code and copy-pasting it into whatever his current project was.  It is a testament, I suppose, to his diligence that most of those project did eventually work -- or at least look like they did.  It's hard to tell.

    My unenviable task, naturally, is to maintain this system.  Wherein I mostly mean rebuild from scratch while preserving the original functionality (or, where the functionality wasn't ... well, functional, to fix it).  I've been doing a lot of "fixing it" but sometimes it's hard to even tell what a block of code was supposed to do.  Naturally, there's zero documentation and the only comments I've found so far are almost Zen in their lack of utility.  Anti-patterns are the rule of the day.  There are no named constants; magic numbers are simply hardcoded whenever they came in handy.  And they come in handy a lot, since much of the data is moved around through giant arrays that are like the inbred cousins of object-oriented structure.

    The creator of this mess was very fond of the for-case construct.  But we've seen that before.  So here's an example of what flow control "should" look like:

    counter3 = -1
    for tempswitch = 3 to 12
        for counter = 0 to ubound(PreOutp, 2)
            index = 0
            counter2 = 0
            if CInt(PreOutp(tempswitch, counter)) <> 0 AND counter < (ubound(PreOutp, 2) - 1) then
                counter2 = counter
                index = index + CInt(PreOutp(tempswitch, counter))
                if CInt(PreOutp(tempswitch, counter2 + 1)) <> 0 then
                    do while (CInt(PreOutp(tempswitch, counter2)) <> 0) AND (PreOutp(0, counter) = PreOutp(0, counter2)) AND counter2 < (ubound(PreOutp, 2) - 1)
                        counter2 = counter2 + 1
                        if (PreOutp(0, counter) = PreOutp(0, counter2)) then index = index + CInt(PreOutp(tempswitch, counter2))
                    loop
                end if
                if (tempswitch = 4) or (tempswitch = 5) then
                    typeswitch = tempswitch - 3
                else
                    typeswitch = tempswitch + 7
                end if
            end if
            if ((index > 300) OR ((typeswitch <> 1) AND (typeswitch <> 2))) AND (counter2 > 0) then
                counter3 = counter3 + 1
                redim preserve Outp(4, counter3)
                Outp(0, counter3) = typeswitch
                Outp(1, counter3) = FormatDateTime(CDate(PreOutp(1, counter)), 3)
                if counter <> (counter2 - 1) then
                    Outp(1, counter3) = DateAdd("s", (900 - CInt(PreOutp(tempswitch, counter))), Outp(1, counter3))
                end if
                Outp(2, counter3) = DateAdd("s", index, Outp(1, counter3))
                Outp(3, counter3) = index
                Outp(4, counter3) = PreOutp(0, counter)
            end if
            if counter2 > counter then counter = counter2
        next
    next

    For the  record, I don't have the foggiest notion what this was supposed to do. 



  • @Serpentes said:

    counter3 = -1
    for tempswitch = 3 to 12

        for counter = 0 to ubound(PreOutp, 2)

            //Snip

            if counter2 > counter then counter = counter2

    Phew, for a second I thought I saw a < sign there. Okay, this isn't much better, but still a bit.



  • Is PreOutP an array or a function call?  I hope it's just an array, but I can't realy tell.

     As for what it's supposed to do the answer is obvious.  It's supposed to drive anyone who looks at it insane (if they aren't already there).



  • It's an array.  It's always an array.  Despite the fact that there are probably tens of thousands of lines of code scattered among the various modules of the projectspace, there are no locally-defined functions, no matter how often a code block gets reused.  My working theory is that the original coder did not know how functions worked.  Although that's a WTF all its own, I'm actually thankful for it.  It means there won't be any lurking horrors in some concealed function block or included file.  No, none of these horrors lurk.

    I do think these loops might be intended to destroy sanity, though.  The code I quoted above is followed by about 1300 lines doing something entirely unrelated before returning to Outp:

    for counter = 0 to ubound(Inp2, 2)
        temp = Inp2(1, counter)
        temp2 = Inp2(2, counter)
        counter2 = counter
        subparse = true
        do while subparse
            counter2 = counter2 + 1
            if counter2 > ubound(Inp2, 2) then
                subparse = false
            else
                if (Inp2(0, counter) = Inp2(0, counter2)) then
                    if (Inp2(1, counter2) - temp2) < (21600) then
                        temp2 = Inp2(2, counter2)
                    else
                        subparse = false
                    end if
                else
                    subparse = false
                end if
            end if
        loop
        counter3 = counter3 + 1
        redim preserve Outp(4, counter3)
        Outp(0, counter3) = 3
        Outp(2, counter3) = DateAdd("s",temp,CDate("01/01/1970"))
        Outp(4, counter3) = Inp2(0, counter)
        if temp2 > 0 then
            counter3 = counter3 + 1
            redim preserve Outp(4, counter3)
            Outp(0, counter3) = 4
            Outp(2, counter3) = DateAdd("s",temp2,CDate("01/01/1970"))
            Outp(4, counter3) = Inp2(0, counter)
        end if
        if counter2 > ubound(Inp2, 2) then exit for
        if counter2 > counter then
            counter = counter2 - 1
        end if
    next

    That's the same Outp as from the previous code snippet and the same counter3, but a new counter and counter2.  Here, totally different data from a totally different data source is appended to the Frankenstein array via yet another bastardized loop structure.  As far as I can tell, Outp(0,n) is some sort of control flag that probably tells something else what kind of data is contained in that row of the array.  That's my guess, anyway, because the values assigned in this codeblock (the magic numbers 3 or 4!) are different from any result that the tempswitch/typeswitch nonsense in the first section can produce.  Maybe.  Either that or its a signal to whatever Outer God should have to put up with this code.

    Oh, and as a side note about the array Inp2: there is no Inp or Inp1 anywhere in any of the modules.



  • @Serpentes said:

    It's an array.  It's always an array.  Despite the fact that there are probably tens of thousands of lines of code scattered among the various modules of the projectspace, there are no locally-defined functions, no matter how often a code block gets reused.  My working theory is that the original coder did not know how functions worked.  Although that's a WTF all its own, I'm actually thankful for it.  It means there won't be any lurking horrors in some concealed function block or included file.  No, none of these horrors lurk.

    I do think these loops might be intended to destroy sanity, though.  The code I quoted above is followed by about 1300 lines doing something entirely unrelated before returning to Outp:

    for counter = 0 to ubound(Inp2, 2)
        temp = Inp2(1, counter)
        temp2 = Inp2(2, counter)
        counter2 = counter
        subparse = true
        do while subparse
            counter2 = counter2 + 1
            if counter2 > ubound(Inp2, 2) then
                subparse = false
            else
                if (Inp2(0, counter) = Inp2(0, counter2)) then
                    if (Inp2(1, counter2) - temp2) < (21600) then
                        temp2 = Inp2(2, counter2)
                    else
                        subparse = false
                    end if
                else
                    subparse = false
                end if
            end if
        loop
        counter3 = counter3 + 1
        redim preserve Outp(4, counter3)
        Outp(0, counter3) = 3
        Outp(2, counter3) = DateAdd("s",temp,CDate("01/01/1970"))
        Outp(4, counter3) = Inp2(0, counter)
        if temp2 > 0 then
            counter3 = counter3 + 1
            redim preserve Outp(4, counter3)
            Outp(0, counter3) = 4
            Outp(2, counter3) = DateAdd("s",temp2,CDate("01/01/1970"))
            Outp(4, counter3) = Inp2(0, counter)
        end if
        if counter2 > ubound(Inp2, 2) then exit for
        if counter2 > counter then
            counter = counter2 - 1
        end if
    next

    That's the same Outp as from the previous code snippet and the same counter3, but a new counter and counter2.  Here, totally different data from a totally different data source is appended to the Frankenstein array via yet another bastardized loop structure.  As far as I can tell, Outp(0,n) is some sort of control flag that probably tells something else what kind of data is contained in that row of the array.  That's my guess, anyway, because the values assigned in this codeblock (the magic numbers 3 or 4!) are different from any result that the tempswitch/typeswitch nonsense in the first section can produce.  Maybe.  Either that or its a signal to whatever Outer God should have to put up with this code.

    Oh, and as a side note about the array Inp2: there is no Inp or Inp1 anywhere in any of the modules.

     

    So what are the data types that wind up in Outp?



  • That's an interesting question, isn't it?  Nothing is ever explictly typed, nor is there ever a comment that even remotely implies what these values mean or should hold.

    There are 5 columns in the table.  Zero, three, and four are always integers when they are populated.  One and two usually have dates assigned to them, but there's a code block where they are cast to integers to compare to some magic values.  Meanwhile, columns one and three are sometimes just left uninitialized.  I'm not entirely sure how they avoid generating errors, actually...

    They do get data, eventually, kinda:

    Outp(r1,r2) = Trim(Outp(r1,r2) & " ")

     



  • Oh My Fairy Godmother, I wish this was unusual... but it isn't. I'm doing a pile of maintenance work on some old ASP websites (for my sins, no doubt) and this sort of thing seems to be the norm.

    One page I struck on one website had lots of array manipulation, just like this, of data pulled from recordsets "for optimisation", but then used a custom regex-based function to emulate Server.URLEncode() -- adding 7 seconds to each page load; replacing that with Server.URLEncode() dropped page loads back under a second :(



  • @Serpentes said:

    That's an interesting question, isn't it?  Nothing is ever explictly typed, nor is there ever a comment that even remotely implies what these values mean or should hold.

    There are 5 columns in the table.  Zero, three, and four are always integers when they are populated.  One and two usually have dates assigned to them, but there's a code block where they are cast to integers to compare to some magic values.  Meanwhile, columns one and three are sometimes just left uninitialized.  I'm not entirely sure how they avoid generating errors, actually...

    They do get data, eventually, kinda:

    Outp(r1,r2) = Trim(Outp(r1,r2) & " ")

     

     

    There isn't an On Error Resume Next is there?



  • No "On Error Resume Next".  I am thankful for small favors. On the other hand, the real problem is that terrible ideas don't generate errors; that Trim line is used to make the uninitialized array cells equal to empty strings.  Except that's not how initialization works.... Really, pretty much anything this code does to avoid generating errors is worse than crashing. I finally found what passes for error checking on the dates created by those horrible loops.  By the way, none of these variables are anonymized and the formatting is as I found it, complete with the swapping between "if' and "If" on alternating lines in this section.

    if Outp(1, counter) <> "" then
        If (Outp(0,counter)<3 or Outp(0,counter)>4) then Outp(1, counter) = d_temptest & " " & Outp(1, counter)
    end if
    if Outp(2, counter) <> "" then
        if Outp(2, counter) = "12/31/1899" then
            Outp(2, counter) = DateAdd("d",1,d_temptest) & " 12:00:00 AM"
        else
            If (Outp(0,counter)<3 or Outp(0,counter)>4) then Outp(2, counter) = d_temptest & " " & Outp(2, counter)
        end if
    end if

    Because discovering that your array contains dates from the 19th century isn't a sign that your algorithm is FUBAR, it just means it's time for a magic value assignment, right?  Oh, and d_temptest?  That's apparently derived from a date passed into this page from another page in projectspace via POST.  I'm sure that nothing can possibly go wrong with this cunning plan.

    dsel1 = request.form("todsel2")
    if dsel1 = "" then
        dsel1 = "[NONE]"
    else
        tempdateARR = split(dsel1,"/")
        fixed = DateSerial(tempdateARR(2),tempdateARR(0),tempdateARR(1))
    end if
    d_temptest = fixed



  • @Serpentes said:

    No "On Error Resume Next".  I am thankful for small favors. On the other hand, the real problem is that terrible ideas don't generate errors; that Trim line is used to make the uninitialized array cells equal to empty strings.  Except that's not how initialization works.... Really, pretty much anything this code does to avoid generating errors is worse than crashing. I finally found what passes for error checking on the dates created by those horrible loops.  By the way, none of these variables are anonymized and the formatting is as I found it, complete with the swapping between "if' and "If" on alternating lines in this section.

    if Outp(1, counter) <> "" then
        If (Outp(0,counter)<3 or Outp(0,counter)>4) then Outp(1, counter) = d_temptest & " " & Outp(1, counter)
    end if
    if Outp(2, counter) <> "" then
        if Outp(2, counter) = "12/31/1899" then
            Outp(2, counter) = DateAdd("d",1,d_temptest) & " 12:00:00 AM"
        else
            If (Outp(0,counter)<3 or Outp(0,counter)>4) then Outp(2, counter) = d_temptest & " " & Outp(2, counter)
        end if
    end if

    Because discovering that your array contains dates from the 19th century isn't a sign that your algorithm is FUBAR, it just means it's time for a magic value assignment, right?  Oh, and d_temptest?  That's apparently derived from a date passed into this page from another page in projectspace via POST.  I'm sure that nothing can possibly go wrong with this cunning plan.

    dsel1 = request.form("todsel2")
    if dsel1 = "" then
        dsel1 = "[NONE]"
    else
        tempdateARR = split(dsel1,"/")
        fixed = DateSerial(tempdateARR(2),tempdateARR(0),tempdateARR(1))
    end if
    d_temptest = fixed

     

    I'm sure his logic was that dates from 1900 are much better.   You should, however, be thankful for his inability to grasp functions.  Otherwise he might have rolled his own DateAdd.  



  • @DescentJS said:

    As for what it's supposed to do the answer is obvious.  It's supposed to drive anyone who looks at it insane (if they aren't already there).

    Thank you, Piers Anthony.  After reading Macroscope, I decided to train my mind to safeguard myself against catatonia-inducing patterns.  That training came in very handy when looking at this code.  (The code may not have induced catatonia otherwise, but the training was still useful anyway.)



  •  Somehow this code reminds me of that machine E.T. built in the movie.

    "It's doing ... something..."



  • I think this poses an interesting challenge: writing an algorithm that searches google for code snippets and constructs a program that compiles and works according to your specifications. Perhaps though an iterative process where the code just gets more and more insane each time the algorithm is run.



  •  They already have that.  It's called: "Outsource to India".



  • @DescentJS said:

    They already have that.  It's called: "Outsource to India".

    Zing!



  • @morbiuswilters said:

    @DescentJS said:

    They already have that.  It's called: "Outsource to India".

    Zing!

     

    That's not funny.

    It's true.



  • I'm not sure any Indian import code has ever been like this, though. So it turns out that at least one of the mega-arrays used to provide the initial values for those mutant loops comes from a totally undocumented but otherwise seemingly sane table in a SQL database.  Unfortunately, database tables don't update themselves ... normally.  This one is populated via a vb6 "application" backended to an Access database on a support server, scripted to load automatically at set intervals.  He was, clearly, no better a vb6 programmer.  In some ways, he was worse.  In vb6, you see, he has functions.  Oh, yes.  Yes, he has functions.

    First off, most of the function names clearly imply they started life as event triggers for some poor, unloved form.  There's no trace of the form now, so any hope to find button labels or vain hints at purpose are moot.  They call a lot of horrifying SQL queries which are sadly just barely self-documenting enough that I'd have to anonymize them, and I think that would take away from the purity of the other code snippets.  The variable declarations, though ... those are pretty good.

    Public Sub Command15_Click()
    Dim SQLstr As String
    Dim TempString1 As String
    Dim TempDate1 As Date
    Dim TempCounter1 As Integer
    Dim TempCounter2 As Integer
    Dim TempVariant1 As Variant


    Followed immediately by...

    Public Sub Command85_Click()
    Dim SQLstr As Variant
    Dim TempCounter1 As Integer
    Dim TempCounter2 As Integer
    Dim TempCounter3 As Integer
    Dim TempString1 As String

    But of course, data type abuses in prefabricated but repurposed control functions would just be bad.  This is worse.  Even this programmer knew about the dangers of SQL injection.  But don't worry!  He makes sure all this data is clean.  So very, very clean.

    Public Function DropSingleQuotes(instring) As Variant
    Dim CheckedOut As Boolean
    Dim WhereAt As Integer
    Dim Cycler As Integer
    Dim TempStringL As String
    Dim TempStringR As String
    CheckedOut = False
    Do While CheckedOut = False
        If InStr(instring, "'") > 0 Then
            WhereAt = InStr(instring, "'")
            If WhereAt > 1 Then TempStringL = Left(instring, (WhereAt - 1)) Else TempStringL = ""
            If Len(instring) > 2 Then TempStringR = Right(instring, (Len(instring) - WhereAt)) Else TempStringR = ""
            instring = TempStringL & TempStringR
            If InStr(instring, "'") = 0 Then CheckedOut = True
        Else
            CheckedOut = True
        End If
    Loop
    DropSingleQuotes = instring
    End Function

    There are several others in this function family.  They all get called, a lot, although most of the variables that get sanitized manage to miss one or more steps, largely at random, although at this point I wonder if that's not just a misguided effort to show mercy to the data.

    Public Function DropDoupleQuotes(instring) As Variant
    Public Function DropSlash(instring) As Variant
    Public Function DropBackSlash(instring) As Variant
    Public Function DropLessThan(instring) As Variant
    Public Function DropGreaterThan(instring) As Variant
    Public Function DropSemiColon(instring) As Variant
    Public Function DropCloseParen(instring) As Variant

    That list is inclusive, by the way.  There is no DropOpenParen, but that's okay, because DropCloseParen actually drops opening parentheses and not closing ones. I despair rebuilding this whole disaster.  But at least this time he's got a fool-proof error handling system.

    On Error Resume Next



  •  Obviously, this programmer didn't understand one fundamentally important concept of engineering:  use short variable names.  They're more efficient than long variable names.  It's much better to do:

    Public Function DSQ(n) As Variant
    Dim co As Boolean
    Dim wa As Integer
    Dim c As Integer
    Dim l As String
    Dim r As String

    The original would have been substantially slower.  It's worth sacrificing maintenance time to have a fast, high-quality product.

     Fortunately, this programmer knew how much comments impact performance as well.



  • @dhromed said:

    @morbiuswilters said:

    @DescentJS said:

    They already have that.  It's called: "Outsource to India".

    Zing!

     

    That's not funny.

    It's true.

    Can't it be both?


Log in to reply