VB.NET -> Cutting down on If statements

This is more of a learning exercise as the code works but i wanted to know if i could have done it with less code or better logic

Function Getthescores(ByVal whatwebpage As String, ByVal whattoloop As String, ByVal hometeam As String, ByVal awayteam As String, ByVal setthetime As String, ByVal homescore As String, ByVal awayscore As String) As String
        Dim thescorestext As String
        Dim webGet As New HtmlWeb() 'open the system
        Dim htmlDoc As HtmlDocument = webGet.Load(whatwebpage) '' get the html from the webpage

        Dim coll As HtmlNodeCollection = htmlDoc.DocumentNode.SelectNodes(whattoloop)
        ' do stuff
        If coll IsNot Nothing Then

            For Each div As Object In coll ' select all the divs within the code that contain *

                If Not div.selectSingleNode(hometeam) Is Nothing Then
                    'set the time to make sure it can be orderd

                    Dim settime As String = System.Text.RegularExpressions.Regex.Replace(div.selectSingleNode(setthetime).InnerText, "[^a-zA-Z0-9]", " ")

                    If settime <> "" Then
                        settime = settime.Substring(1, 2)
                    End If

                    Dim orderbytime As Integer
                    If settime = "HT" Then
                        orderbytime = 45
                    ElseIf settime = "FT" Then
                        orderbytime = 999
                    Else
                        orderbytime = Convert.ToInt32(settime)
                    End If
                            thescorestext = ""

                    thescorestext += StringOutNumbers(div.selectSingleNode(hometeam).InnerText) 'get home team name
                  
                    thescorestext += div.selectSingleNode(homescore).InnerText & " - " & div.selectSingleNode(awayscore).InnerText

                    thescorestext += StringOutNumbers(div.selectSingleNode(awayteam).InnerText) ' get away teams name

                    Dim count = scorestable.Rows.Cast(Of DataRow)().Where(Function(n) n("HomeTeam").ToString() = StringOutNumbers(div.selectSingleNode(hometeam).InnerText)).Count()
                    'Find the count
                    If count = 0 Then
                        'if not finished add row to the datatable as no results where found

                        scorestable.Rows.Add(StringOutNumbers(div.selectSingleNode(hometeam).InnerText), settime, thescorestext, "", "", orderbytime)
                    Else
                        'A matching result was found so now check to see if the Scores section of the result matches the value - thescorestext
                        For Each row As DataRow In scorestable.Rows
                            If row("HomeTeam").ToString() = StringOutNumbers(div.selectSingleNode(hometeam).InnerText) And row("Score").ToString() <> thescorestext Then
                                'add scores
                                row.SetField("Score", thescorestext)
                                row.SetField("Time", settime)
                                row.SetField("Lastgoaltime", DateTime.Now())
                                row.SetField("Kickoff", orderbytime)
                            ElseIf row("HomeTeam").ToString() = StringOutNumbers(div.selectSingleNode(hometeam).InnerText) And row("Score").ToString() = thescorestext Then
                                ' add time
                                row.SetField("Time", settime)
                                row.SetField("Kickoff", orderbytime)
                            End If
                        Next
                    End If
                End If
            Next
        End If
        Return thescorestext


    End Function

Open in new window

runnerjp2005Asked:
Who is Participating?
 
ElrondCTConnect With a Mentor Commented:
One change I'd suggest for your scorestable loop:
                        For Each row As DataRow In scorestable.Rows
                            If row("HomeTeam").ToString() = StringOutNumbers(div.selectSingleNode(hometeam).InnerText) Then
                                If row("Score").ToString() <> thescorestext Then
                                    'add scores
                                    row.SetField("Score", thescorestext)
                                    row.SetField("Lastgoaltime", DateTime.Now())
                                End If
                                ' add time
                                row.SetField("Time", settime)
                                row.SetField("Kickoff", orderbytime)
                            End If
                        Next

Open in new window

When possible, avoid testing for the same thing twice, and setting the same thing twice. You may want to create a temporary variable holding the value of StringOutNumbers(div.selectSingleNode(hometeam).InnerText), since you use that several times.
0
 
Guy Hengel [angelIII / a3]Billing EngineerCommented:
no, I don't see any way to "improve" this, except by adding comments  ...
Dim coll As HtmlNodeCollection = htmlDoc.DocumentNode.SelectNodes(".//li[@class='avb-row IN_PLAY']")
		' do stuff
If coll IsNot Nothing Then

        For Each div As Object In htmlDoc.DocumentNode.SelectNodes(".//li[@class='avb-row IN_PLAY']") ' select all the divs within the code that contain *

         If Not div.selectSingleNode(".//span[@class='home-team-name']") Is Nothing Then
                'set the time to make sure it can be orderd



                Dim settime As String = System.Text.RegularExpressions.Regex.Replace(div.selectSingleNode(".//span[@class='event-inplay-state inplay ui-time-stop-format ']").InnerText, "[^a-zA-Z0-9]", " ")

                If settime <> "" Then
                    settime = settime.Substring(1, 2)
                End If

                Dim orderbytime As Integer
                If settime = "HT" Then
                    orderbytime = 45
                ElseIf settime = "FT" Then
                    orderbytime = 999
                Else
                    orderbytime = Convert.ToInt32(settime)
                End If


                '            Dim Scoresplit() As String = div.selectSingleNode(".//td[@class='score']").InnerText.Split("-") 'split the string for scores
                thescorestext = ""
                '            '' thescorestext += div.selectSingleNode(".//td[@class='time']").InnerText.Trim().Substring(0, 2) & "" 'set the time
                thescorestext += StringOutNumbers(div.selectSingleNode(".//span[@class='home-team-name']").InnerText) 'get home team name

                '            ' lblHTMLOutput.Text += mostused.gettheteams(StringOuttext(div.selectSingleNode(".//td[@class='home]").InnerText)) -redcard?

                '            'set the score with 0 being home team and 1 being away
                thescorestext += div.selectSingleNode(".//span[@class='ui-score-home']").InnerText & " - " & div.selectSingleNode(".//span[@class='ui-score-away']").InnerText

                '            '  lblHTMLOutput.Text += mostused.gettheteams(StringOuttext(div.selectSingleNode(".//td[@class='away']").InnerText))

                thescorestext += StringOutNumbers(div.selectSingleNode(".//span[@class='away-team-name']").InnerText) ' get away teams name

                '            'Check to see if anyrows in HomeTeam contain a value
                Dim count = scorestable.Rows.Cast(Of DataRow)().Where(Function(n) n("HomeTeam").ToString() = StringOutNumbers(div.selectSingleNode(".//span[@class='home-team-name']").InnerText)).Count()
                'Find the count
                If count = 0 Then
                    'if not finished add row to the datatable as no results where found
                    '  scorestable.Rows.Add(StringOutNumbers(div.selectSingleNode(".//span[@class='home-team-name']").InnerText), "", thescorestext, "", "", "")


                    scorestable.Rows.Add(StringOutNumbers(div.selectSingleNode(".//span[@class='home-team-name']").InnerText), settime, thescorestext, "", "", orderbytime)

                Else  ' If count = 0 Then
                    'A matching result was found so now check to see if the Scores section of the result matches the value - thescorestext
                    For Each row As DataRow In scorestable.Rows
                        If row("HomeTeam").ToString() = StringOutNumbers(div.selectSingleNode(".//span[@class='home-team-name']").InnerText) And row("Score").ToString() <> thescorestext Then
                            'add scores
                            row.SetField("Score", thescorestext)
                            row.SetField("Time", settime)
                            row.SetField("Lastgoaltime", DateTime.now())
                            row.SetField("Kickoff", orderbytime)
                        ElseIf row("HomeTeam").ToString() = StringOutNumbers(div.selectSingleNode(".//span[@class='home-team-name']").InnerText) And row("Score").ToString() = thescorestext Then

                            ' add time
                            row.SetField("Time", settime)
                            row.SetField("Kickoff", orderbytime)

                        End If  ' else if  row("HomeTeam").ToString()  ...

                    Next ' For Each row As DataRow In scorestable.Rows

                End If ' else if  If count = 0 Then
            End If '    If Not div.selectSingleNode(".//span[@class='home-team-name']") Is Nothing 

        Next  ' For Each div As Object In 
	end if  ' If coll IsNot Nothing Then

        Return thescorestext
                                  

Open in new window


this will make your code more readable.
you might also use regions directives to be able to "collapse" code regions
http://msdn.microsoft.com/en-us/library/sd032a17.aspx
0
 
käµfm³d 👽Commented:
Don't be afraid to add more function definitions when you have functions that start to get unwieldy. Having to scroll down 2 or three pages to see the rest of a block of code becomes tiring; so is scrolling around to try and locate a variable's declaration. Just make sure that functions you create make sense and logically encapsulate a piece of logic--don't create a new function solely for the sake of having another function have less lines.
0
 
John ClaesSenior .Net Consultant & Technical AnalistCommented:
Both previous comments are correct. I would suggest looking at them both ;-)

the foreach loop seems for me a legit function extraction possibility
the comments are a common issue, but try to write at least for every 2-3 lines 1 line of comment. It's hard (time consuming) for you now but easy for the future and the time-profit can be easly ten times

regards
0
All Courses

From novice to tech pro — start learning today.