8 thoughts on “CFB Stats Refactor”

1. Consider:

   Public Function IsAway(hRow As HTMLTableRow) As Boolean
      
       Dim bReturn As Boolean
      
       bReturn = Left$(hRow.Cells(1).innerText, 1) = “@”
      
       If Left$(hRow.Cells(1).innerText, 1) = “+” Then
           bReturn = Left$(hRow.Cells(3).innerText, 1) = “L”
       End If
      
       IsAway = bReturn
      
   End Function

   When I see code like this, here’s how my thinking goes. Clearly bReturn is getting a true/false value with the 1st assignment statement. But, since the code continues, I expect it to potentially get changed. But, that change is possible only if the first assignment yields a false value. That is not obvious from the code.

   Below are two possible alternatives. The first would be my preferred approach. In VBA it differs from your code in that it requires Cell(3).innerText to exist irrespective of the content of Cells(1). But as long as that condition is satisfied, the code is, IMO, more obvious.

       IsAway = Left$(hRow.Cells(1).innerText, 1) = “@” _
           Or (Left$(hRow.Cells(1).innerText, 1) = “+” _
               And Left$(hRow.Cells(3).innerText, 1) = “L”)

   The other would be to make the conditional nature more obvious

       If Left$(hRow.Cells(1).innerText, 1) = “@” Then
           bReturn = True
       ElseIf Left$(hRow.Cells(1).innerText, 1) = “+” Then
           bReturn = Left$(hRow.Cells(3).innerText, 1) = “L”
           End If

   And, yes, it’s possible to evaluate the 1st term only once — either with a temporary variable or better yet a Select Case.

2. Here’s something else to consider:

                               If bOffense Then
                                   With clsGame
                                       If bIsAway Then
                                           .AwayRushYards = hRow.Cells(4).innerText
                                           .AwayPassYards = hRow.Cells(5).innerText
                                           .AwayPlays = hRow.Cells(6).innerText
                                       Else
                                           .HomeRushYards = hRow.Cells(4).innerText
                                           .HomePassYards = hRow.Cells(5).innerText
                                           .HomePlays = hRow.Cells(6).innerText
                                       End If
                                   End With
                               Else
                                   With clsGame
                                       If bIsAway Then
                                           .HomeRushYards = hRow.Cells(4).innerText
                                           .HomePassYards = hRow.Cells(5).innerText
                                           .HomePlays = hRow.Cells(6).innerText
                                       Else
                                           .AwayRushYards = hRow.Cells(4).innerText
                                           .AwayPassYards = hRow.Cells(5).innerText
                                           .AwayPlays = hRow.Cells(6).innerText
                                       End If

   I believe the below is equivalent to the above. And, of course, one can dispense with the Defense and AtHome booleans.

                               dim bDefense as Boolean: bDefence=not bOffence
                               dim bAtHome as boolean: atHome=not bisAway
                               With clsGame
                               If (bOffense and bIsAway) or (bDefense and bAtHome) Then
                                   .AwayRushYards = hRow.Cells(4).innerText
                                   .AwayPassYards = hRow.Cells(5).innerText
                                   .AwayPlays = hRow.Cells(6).innerText
                               Else
                                   .HomeRushYards = hRow.Cells(4).innerText
                                   .HomePassYards = hRow.Cells(5).innerText
                                   .HomePlays = hRow.Cells(6).innerText
                                   End If
                                   End With

3. This will NOT allow Nebraska to be ranked any higher in the polls. :)

4. Next to consider…

   Lose the deeply embedded block of code that creates a new Game object. Modularize the new game creation. Instead of

                               If clsGame Is Nothing Then
                                   Set clsGame = New CGame
                                   With clsGame
                                       .GameDate = DateValue(hRow.Cells(0).innerText)
                                       .SetScore hRow.Cells(3).innerText, bIsAway
                                       If bIsAway Then
                                           Set .HomeTeam = clsOpponent
                                           Set .AwayTeam = clsTeam
                                       Else
                                           Set .HomeTeam = clsTeam
                                           Set .AwayTeam = clsOpponent
                                       End If
                                   End With
                                   Me.Add clsGame
                                   clsTeam.Games.Add clsGame
                                   clsOpponent.Games.Add clsGame
                               End If

   use

   function NewGame(byref clsTeam as …,byref clsOpponent as …, _
           hRow as …, bIsAway as boolean)as CGame
       dim clsGame as CGame
       Set clsGame = New CGame
       With clsGame
       .GameDate = DateValue(hRow.Cells(0).innerText)
       .SetScore hRow.Cells(3).innerText, bIsAway
       If bIsAway Then
           Set .HomeTeam = clsOpponent
           Set .AwayTeam = clsTeam
       Else
           Set .HomeTeam = clsTeam
           Set .AwayTeam = clsOpponent
           End If
           End With
       Me.Add clsGame
       clsTeam.Games.Add clsGame
       clsOpponent.Games.Add clsGame
       set NewGame=clsGame
       End Function

   and in the nested loop,

                               If clsGame Is Nothing Then set clsGame=NewGame(…)

5. Following in Tushar’s footsteps, I believe the following (untested) code is another way to “reduce” your IsAway function…

   Public Function IsAway(hRow As HTMLTableRow) As Boolean
       IsAway = Left$(hRow.Cells(1).innerText, 1) = “+”
       IsAway = Left$(hRow.Cells(1 – 2 * IsAway).innerText, 1) = Mid(“@L”, 1 – IsAway, 1)
   End Function

   which, if desired, can be turned into a one-liner with a simple substitution…

   Public Function IsAway(hRow As HTMLTableRow) As Boolean
     IsAway = Left$(hRow.Cells(1 – 2 * (Left$(hRow.Cells(1).innerText, 1) = “+”)). _
         innerText, 1) = Mid(“@L”, 1 – (Left$(hRow.Cells(1).innerText, 1) = “+”), 1)
   End Function

6. One more issue to consider. Whenever I see a series of nested If / loop constructs I dread what is coming next. Luckily, in this case there were no Else clauses because that can be prescription for a disaster.

   In any case, I believe the 6 levels of nesting can be reduced to 3.

       For Each hTbl In hDoc.all.tags(sTBLTYPE)
           If hTbl.className = sTBLCLASS Then
               For Each hRow In hTbl.Rows
                   If hRow.RowIndex > 0 Then
                       If hRow.Cells(0).className = sDTECLASS Then
                           If hRow.Cells(1).Children.Length > 0 Then

       For Each hTbl In hDoc.getElementsByTagName(sTBLTYPE) _
               .getElementsByClassName(sTBLCLASS)
           dim RowIdx as integer
           for rowidx=1 to htbl.rows.length -1
               set hRow=htbl.rows(rowidx)
               If hRow.Cells(0).className = sDTECLASS _
                       and hRow.Cells(1).Children.Length > 0 Then
                   ‘existing code

   A reference for the outermost For: https://developer.mozilla.org/en/DOM/document.getElementsByClassName

   As in a previous comment, the last If…And requires that Cells(1) exists irrespective of the class name of Cells(0). If not, VBA, since it does not short-circuit its boolean operations, will fault.

   What I do in a case like this is to create a boolean function for the nested test or *not* indent the 2nd If, thereby letting the reader know that the structure is *not* a real nested clause.

   So, instead of

                       If hRow.Cells(0).className = sDTECLASS Then
                           If hRow.Cells(1).Children.Length > 0 Then

   use

                       If hRow.Cells(0).className = sDTECLASS Then
                       If hRow.Cells(1).Children.Length > 0 Then
                           ‘existing code
                          End If
                           End If

7. Rick, Thanks for putting the word reduce in your comment in quotes {grin}. My suggestion was *not* to simply reduce the number of lines of code. The idea is to also make the code easier to read, understand, and maintain.

   From an abstract perspective, it is interesting to read how you formulated your approach. However, it requires duplicate code fragments {shudder, shudder} and implicit data type conversions {shudder, shudder}. It also requires that one test the 1st character of both cells. While that is currently true, it might not be in the future and writing code so intimately tied to the existing data structure / layout is, IMO, not for maintainability.

8. @Tushar,

   In response to your comments, the main reason behind what I posted was… “It was Sunday and I was bored.”{big grin} However, I would like to respond to your specific points.

   > From an abstract perspective, it is interesting to read how you formulated your approach.

   I have noticed over the several years of my answering questions on newsgroups and forums that I tend to come up with code solutions that differ in approach from what almost everyone else posts… probably a brain dysfunction on my part or something.{smile}

   > However, it requires duplicate code fragments {shudder, shudder}

   My two-line solution doesn’t use duplicated code fragments (the one-liner was just offered because I know there are fans of one-liners out there)

   > and implicit data type conversions {shudder, shudder}.

   If you are referring to the use of a Boolean in a multiplication, then I disagree. The documentation for Boolean Data Type in the help files says that this data type is “stored as 16-bit (2-byte) numbers” with only two possible values and then it goes on to say that “When Boolean values are converted to other data types, False becomes 0 and True becomes -1?. In addition, the help files for the True keyword says “The True keyword has a value equal to -1 and, correspondingly, “The False keyword has a value equal to 0?. So, the conversion behavior is not only well documented, it is a fully expected behavior as well.

   > It also requires that one test the 1st character of both cells. While that is currently
   > true, it might not be in the future and writing code so intimately tied to the existing
   > data structure / layout is, IMO, not for maintainability.

   While I would not expect these particular encoding symbols to change given their purpose, your point about maintainability is well taken. Now, while I would have no trouble patching my own code as needed for any future possible changes (its logic is pretty obvious to me personally), I can see where others might have to scratch their heads awhile before being able to make changes to it. And, of course, your suggested code is much easier to read; but I will remind you, my code was not really meant as a serious alternative (which I should have made clearer initially); rather, to repeat, it was Sunday and I was bored”.{very big grin}

Posting code? Use <pre> tags for VBA and <code> tags for inline.