I had some redundant code in my previous post that I’m going to fix up. First, this
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 |
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 If Left$(hRow.Cells(3).innerText, 1) = "L" Then bReturn = True Else bReturn = False End If End If IsAway = bReturn End Function |
Gets changed to this
1 2 3 4 5 6 7 8 9 10 11 12 13 |
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 |
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
1 2 3 4 5 6 7 8 9 10 11 |
Public Sub FillGames() Dim clsTeam As CTeam For Each clsTeam In gclsTeams FillOffenseDefense clsTeam, True FillOffenseDefense clsTeam, False Next clsTeam End Sub |
A little cleaner, I’d say. The new private method looks like this
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89 90 91 92 93 94 95 96 97 |
Private Sub FillOffenseDefense(clsTeam As CTeam, bOffense As Boolean) Dim xmlReq As MSXML2.XMLHTTP Dim hDoc As HTMLDocument Dim hTbl As HTMLTable Dim hRow As HTMLTableRow Dim dtGame As Date Dim clsOpponent As CTeam Dim bIsAway As Boolean Dim clsGame As CGame Dim sUrl As String Const sTBLCLASS As String = "game - log" Const sTBLTYPE As String = "TABLE" Const sDTECLASS As String = "date" Set xmlReq = New MSXML2.XMLHTTP Set hDoc = New HTMLDocument If bOffense Then sUrl = clsTeam.OffenseUrl Else sUrl = clsTeam.DefenseUrl End If xmlReq.Open "GET", sUrl, False xmlReq.send hDoc.body.innerHTML = xmlReq.responseText 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 dtGame = DateValue(hRow.Cells(0).innerText) Set clsOpponent = gclsTeams.TeamByName(hRow.Cells(1).innerText) Set clsGame = gclsGames.FindGameByDateAndTeams(dtGame, clsTeam.TeamName, clsOpponent.TeamName) bIsAway = IsAway(hRow) 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 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 End With End If bIsAway = False End If End If End If Next hRow End If Next hTbl End Sub |
Eliminating redundant code makes it easier to read and understand and helps to prevent errors when making changes.
Consider:
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.
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
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.
Here’s something else to consider:
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 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
This will NOT allow Nebraska to be ranked any higher in the polls. :)
Next to consider…
Lose the deeply embedded block of code that creates a new Game object. Modularize the new game creation. Instead of
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
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,
Following in Tushar’s footsteps, I believe the following (untested) code is another way to “reduce” your IsAway function…
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…
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
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.
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
.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(1).Children.Length > 0 Then
use
If hRow.Cells(1).Children.Length > 0 Then
‘existing code
End If
End If
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.
@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}