Collect and calculate average times from log, then display top 10 longest durations

Question

Here's a novel-length summary of the issue:

I'm trying to write a VB.net program to help me collect remote site statistics from system-generated logs, but I'm a little like a carpenter who only knows how to use a hammer, and my project has turned into a bit of a monstrosity; as embarrassing as my code is, I would really love to get some professional opinions on how I can make it more streamlined and efficient, and generally less embarrassing.

Here's the basic rundown of relevant program functionality:

1. The user can select up to five plaintext log files, each of which can be relatively long (the longest I have available for testing is 26k lines).
2. The program reads through every line of each file in turn, using IO.File.ReadLines, looking for relevant entries (in this case, every time a terminal goes UP or DOWN), and records the information in an "entry" object, which is stored in a list of entry objects. (At this point, I do a lot with the entries, but I'm going to focus just on one activity for this question).
3. To find individual site outages, the program reads through the list of entries until it finds the first "DOWN" entry. It records the site ID, the site's group ID, and the outage start time. At this point, things start to get grossly inefficient.
4. After it has collected the information listed in step 3, it records the current entry list index as a bookmark, then proceeds to look through all the following indices until it finds the next entry with that site ID; if that entry has an "UP" status, then it records that entry time as the outage end time, and calculated the total duration of the outage, then it goes back to the bookmark to look for the next outage start time. If it's a "DOWN" status, it scraps the current outage and goes back to the bookmark to look for the next outage start time. All of this information (and that recorded from step 3) is recorded in an "outage" object, and stored in a list of outages. This step takes an extremely long time.
5. The program then goes through the list of outages, and checks to see if the site ID is contained in a dictionary(of string, array). If so, it adds the outage duration to the dictionary value array index 0, and it adds 1 to the array index 1. This way, I can keep track of the total outage duration for that site, and the total number of outages.
6. Once all the outages have been added to the dictionary of sites, the program runs through that dictionary and calculates the average downtime of each site, and puts the results into another dictionary(of string,integer) to associate the site ID with its average downtime. It also adds each average downtime to a list (called "sorter"). This next part is really sloppy, but I don't know how to do it better (or at all).
7. The "sorter" list is then sorted in descending order. When the average outage times are graphed (xval is index, yval is average duration in minutes), it only plots durations greater than the value in sorter(9); my intent was to graph only the top ten sites (by average downtime), because thousands can be present in a single log file and graphing all of them would be unreadable. However, there are many many problems with doing it this way, and I don't know how to do it better when the values are stored in a dictionary. Likewise, I can't store them in a list(of array), because I'd need to store strings and integers in the same array (unless there's a way around mixing types like that?).

Here are the specific questions I have:

1. Is there a more efficient way to perform these searches without resorting to so many time-consuming nested loops?

2. Is there a more efficient and effective way to sort my outages by the outage duration (integer), while still keeping that duration associated with the site ID (string)?

---

Public Sub avgdowntimepersite(ByVal type As DataVisualization.Charting.SeriesChartType)
    Dim stats As New Dictionary(Of String, Integer)
    Dim sorter As New List(Of Integer)
    Dim outages As New List(Of outage)
    Dim sites As New Dictionary(Of String, Array)

    Dim x = 0 'This is a bookmark to return to after finding the start and stop times of an outage.
    For i = 0 To searchedlist.Count - 2
        Dim entry = searchedlist(i)
        Dim newoutage As New outage
        newoutage = Nothing
        If entry.status = "Down" Then               'Find the first "Down" status in the list of search results.
            newoutage.termid = entry.termid         'Gather as much info as you can from the "Down" status.
            newoutage.popid = entry.popid
            newoutage.starttime = entry.dtg
            x = i                                   'Set the bookmark index to the index at which the "Down" status was found.
            For a = i + 1 To searchedlist.Count - 2 'Go to the next line and start searching for the next status for this site.
                Dim findend = searchedlist(a)       'If the searchresult termid matches the termid of the current outage...
                If findend.termid = newoutage.termid And findend.status = "Up" Then     '...and the status is "Up"...
                    newoutage.endtime = findend.dtg                                     '...collect the end time of the outage...
                    newoutage.duration = newoutage.endtime - newoutage.starttime        '...and calculate the duration, in minutes.
                    outages.Add(newoutage)                                              'Finally, add the new outage to the list of outages.
                    i = x + 1                                                           'Go to one line after the bookmark to start looking for the next outage.
                ElseIf findend.termid = newoutage.termid And findend.status = "Down" Then   'If the searchresult termid matches the outage termid, but it's another "Down" status...
                    newoutage = Nothing                                                     '...scrap the current outage as unresolveable...
                    i = x                                                                   '...and go back to the bookmark and start looking for the next outage.
                    Continue For
                End If
            Next
        End If
    Next

    If outages.Count > 0 Then                                           'If there were actually outages found by the above loop...
        sites.Add(outages(0).termid, {outages(0).duration.Minutes, 1})  'Add the first outage to the list of sites. Format is: termid,(total duration, total # of outages)

        For i = 1 To outages.Count - 1
            Dim item As String = outages(i).termid
            If sites.ContainsKey(item) Then                                     'If the current outage is already in the dictionary of sites...
                sites(item)(0) = sites(item)(0) + outages(i).duration.Minutes   '...add the duration of the current outage to the total outage duration for that site...
                sites(item)(1) += 1                                             '...and increase that site's total number of outages by one.
            Else
                sites.Add(item, ({outages(i).duration.Minutes, 1}))
            End If
        Next


    End If

    For Each tml In sites
        stats.Add(tml.Key, (tml.Value(0) / tml.Value(1)))           'Calculate the average duration of each outage, and add it to the "stats" dictionary.
        sorter.Add((tml.Value(0) / tml.Value(1)))
    Next

    sorter.Sort()
    sorter.Reverse()



    If stats.Count > 0 Then
        outagechart.Series.Clear()
        outagechart.Series.Add("avgpersite")
        outagechart.Series(0).ChartType = type
        outagechart.Series(0).Color = Color.Lime

        outagechart.ChartAreas(0).AxisY.LabelStyle.ForeColor = Color.Gold

        outagechart.ChartAreas(0).AxisX.LabelStyle.Angle = 45
        outagechart.ChartAreas(0).AxisX.LabelStyle.ForeColor = Color.Gold
        outagechart.ChartAreas(0).AxisX.Interval = 1
        outagechart.ChartAreas(0).AxisX.IntervalType = DataVisualization.Charting.DateTimeIntervalType.NotSet
    End If

    For Each tml In stats
        If tml.Value > sorter(9) Then
            outagechart.Series(0).Points.AddXY(tml.Key, tml.Value)
            outagechart.Series(0).IsXValueIndexed = True
        End If

    Next



End Sub

Answer 1

It seems to me that the main thing you need to do is sort out your logic.

The way I'd expect things to work would be that for any given site, a DOWN entry indicates that site has gone down, and it's counted as remaining down until you see an UP entry for the same site. If you see a number of DOWN messages for one site without an intervening UP message, it just means the site went down, and remained down.

That does not seem to fit your description though. Based on how you've described the logic, it sounds like when/if you see two DOWN messages for a given site, you (effectively) re-start the search from the latter of the two. That would mean that given N consecutive DOWN entries for one site, you count the site as being down only from the last of these messages until the following UP message (for the same site). If that's the case, you can speed up the search quite a bit by simply keeping track of the most recently-seen DOWN entry for a given site, and when you see an UP message for that site, you then enter it (along with the preceding DOWN message to the outages collection.

Either way, it sounds like your complexity is currently (roughly) O(N²), but can be reduced to O(N), which is likely to give quite a large improvement. I don't think it makes a lot of sense spend much time looking at the code itself until it's clear what logic it really implements, and (especially) that the logic it implements represents a good way to solve your problem.

Answer 2

Aside from the issues identified in @JerryCoffin's answer, I see a number of things in your code:

• Your naming convention seems to be alllowercase... it's consistent, but hard to read. VB.NET standards would call for PascalCasing.
• The procedure's name doesn't say what it's doing: AvgDownTimePerSite would perhaps be a nice name for an IEnumerable(Of Double); good method names start with a verb.
• The method is doing too many things, which makes it harder to read and harder to maintain.

I would try to break it down into multiple, small steps:

• The first For loop gives you your outages data. That's one thing - make that a function that returns an IEnumerable(Of outage).
• In another function, iterate the outages and perform that other looping logic, return sites.
• Compute and return the stats (the chart's data).
  • I'm not sure I understand the sorter and the logic behind If tml.Value > sorter(9).
• Create and populate a chart.
• Format the chart.

The more focused-on-a-single-task a method is, the easier it is to identify the code that needs to change and tell it from the code that can remain untouched.

With more cohesive code, you can more easily identify areas that can be improved, or rewrite parts that need to be reimplemented - the steps remain the same (e.g. "get outages data"), but the implementation can improve - for example you might want to try and see what LINQ can do for you (I do C#, I'm not even going to try LINQ in VB.NET!), to replace the For and For Each loops.

Answer 3

I'm not understanding everything but you might be able to change your first pass into a single for loop by keeping track of things a different way.

Dim newOutage As New outage
Dim lastDownItem As New Dictionary(Of Integer, Outage)

For Each entry In searchedlist
    If entry.status = "Down" Then
        newOutage = New outage

        newoutage.termid = entry.termid
        newoutage.popid = entry.popid
        newoutage.starttime = entry.dtg

        If lastDownItem.ContainsKey(entry.termid) Then
            lastDownItem(entry.termid) = newoutage
        Else
            lastDownItem.Add(entry.termid, newoutage)
        End If
    Else If entry.status = "Up" Then
        If lastDownItem.ContainsKey(entry.termid) Then
            newoutage = lastDownItem(entry.termid)
            lastDownItem.Remove(entry.termid)

            newoutage.endtime = findend.dtg
            newoutage.duration = newoutage.endtime - newoutage.starttime
            outages.Add(newoutage) 
        Else
            ' Error?
        End If
    End If
Next