CFB Stats Refactor

I had some redundant code in my previous post that I’m going to fix up. First, this

Gets changed to this

Whenever I assign True or False to a Boolean in an If block, it probably needs to be refactored.

Next I take that huge FillGames method and factor out the two major blocks into its own sub. Fill Games now looks like this

A little cleaner, I’d say. The new private method looks like this

Eliminating redundant code makes it easier to read and understand and helps to prevent errors when making changes.

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. 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(…)
  4. 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
  5. 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
  6. 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.

  7. @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.

Leave a Reply

Your email address will not be published. Required fields are marked *