Optimize a Date and Timespan function

Question

I have a working function that may be optimizable.

Currently processing 4 objects gives me 0.0459822 for the elapsed time in seconds - used for testing purposes. Once implemented into the app I will be checking > 1000 objects at a time and would like to speed this up if possible. The format of the fd.dataTime and the other TimeSpans are from a database and I have no control over the format. The reason for adding a day to some Dates is due to the time going over midnight and would yield an incorrect minimum timespan as a result.

Required: manually adding a day to the date if the time is less than the date - only case is presented below(before midnight date - after midnight timespans). Only store valid timespans. nullField is a String that = 0:00:00 an empty TimeSpan found in DB.

Here is the Function:

Private Function GetMinTimeSpan(fds As List(Of Field)) As String
 Dim result As New List(Of Date)
 For Each fd In fds
  Dim dt As Date = fd.dateTime
  Dim s1 As TimeSpan = TimeSpan.Parse(dt.ToString("HH:mm:ss"))
  Dim s2 As TimeSpan = TimeSpan.Parse(fd.sg)
  Dim s3 As TimeSpan = TimeSpan.Parse(fd.os)
  If Not fd.sg = nullField Then
    If s1 > s2 Then
      result.Add(Date.Parse(dt.AddDays(1).ToShortDateString & " " & fd.sg))
    Else
      result.Add(Date.Parse(dt.ToShortDateString & " " & fd.sg))
    End If
  End If
  If s1 > s3 Then
    result.Add(Date.Parse(dt.AddDays(1).ToShortDateString & " " & fd.os))
  Else
    result.Add(Date.Parse(dt.ToShortDateString & " " & fd.os))
  End If
 Next
 Return result.Min.ToString("HH:mm:ss")
End Function 

Here is a sample testing:

Dim sw As New Stopwatch
sw.Start()
Dim fd1 As New Field With {.dateTime = Date.Now, .sg = "23:59:00", .os = "00:03:00"}
Dim fd2 As New Field With {.dateTime = Date.Now, .sg = "00:01:00", .os = "00:03:00"}
Dim fd3 As New Field With {.dateTime = Date.Now, .sg = "0:00:00", .os = "00:04:00"}
Dim fd4 As New Field With {.dateTime = Date.Now, .sg = "00:12:00", .os = "00:05:00"}
Dim fds As New List(Of Field) From {fd1, fd2, fd3, fd4}
Dim m = GetMinTimeSpan(fds)
Debug.WriteLine(sw.Elapsed.ToString)

Answer 1

I'm not sure how to optimize your code, but let's talk about those variable names.

Naming is hard. It's doubly hard in vb.net, because it's case insensitive. That fact makes it difficult not to step on keywords like Field. I don't like recommending this, but without a being able to use fields and field to differentiate from the keywords Fields and Field, you could de-vowel it to flds and fld and still have a variable name that's easier to understand than fd.

dt I get, but what does that date actually represent?

s1,s2,s3? What do those mean? I vaguely understand what s1 is, but then I hit this.

Dim s2 As TimeSpan = TimeSpan.Parse(fd.sg)
Dim s3 As TimeSpan = TimeSpan.Parse(fd.os)

Field.sg and Field.os? Why do we have a stargate address and an operating system stored in those fields?

TL;DR: Stop over abbreviating variable names. They make sense to you (now), but will they make sense to Mr. Maintainer? Remember, that poor confused maintainer could be a future version of yourself.

Answer 2

Your throw-away "test code" doesn't need to Dim a variable and introduce YACI (Yet Another Cryptic Identifier) - this:

Dim sw As New Stopwatch
sw.Start()
Dim fd1 As New Field With {.dateTime = Date.Now, .sg = "23:59:00", .os = "00:03:00"}
Dim fd2 As New Field With {.dateTime = Date.Now, .sg = "00:01:00", .os = "00:03:00"}
Dim fd3 As New Field With {.dateTime = Date.Now, .sg = "0:00:00", .os = "00:04:00"}
Dim fd4 As New Field With {.dateTime = Date.Now, .sg = "00:12:00", .os = "00:05:00"}
Dim fds As New List(Of Field) From {fd1, fd2, fd3, fd4}
Dim m = GetMinTimeSpan(fds)
Debug.WriteLine(sw.Elapsed.ToString)

...could be made much easier to tweak the inputs at will if you get rid of the identifiers altogether and simply use a collection initializer - also declaring, initializing and starting the Stopwatch could be a one-liner:

Dim sw As Stopwatch = Stopwatch.StartNew()
Dim testFields As New List(Of Field) From
{
    New Field With {.dateTime = Date.Now, .sg = "23:59:00", .os = "00:03:00"},
    New Field With {.dateTime = Date.Now, .sg = "00:01:00", .os = "00:03:00"},
    New Field With {.dateTime = Date.Now, .sg = "0:00:00", .os = "00:04:00"},
    New Field With {.dateTime = Date.Now, .sg = "00:12:00", .os = "00:05:00"}
}
Dim result = GetMinTimeSpan(testFields)
Debug.WriteLine(sw.Elapsed.ToString)

Now if I want to add/remove a Field to/from the test suite, I can just do that.

I realize this is probably throw-away code. The point is, it's code you've written, and that's how you wrote it. I ran it a couple of times, and this is the string representation printed to the debug output:

00:00:00.0189976
00:00:00.0107947
00:00:00.0120666
00:00:00.0158479
00:00:00.0222599

So I put the method call in a tight loop, running 10K iterations:

    For index = 1 To 10000
        GetMinTimeSpan(fds)
    Next

    Debug.WriteLine(sw.Elapsed.ToString)

And it printed this:

00:00:00.3523980
00:00:00.3053835
00:00:00.3534577
00:00:00.3389172
00:00:00.3423430

Just what are you trying to optimize exactly? Have you identified a bottleneck somewhere? I don't see a performance issue with your code at first glance, because it's pretty hard to get a hold of what your code does, at a glance.

Let's see:

Private Function GetMinTimeSpan(fds As List(Of Field)) As String

To me, a function that's called GetMinTimeSpan should rightfully return a TimeSpan. Yours is returning a String, which is potentially somewhat irritating for the maintainer. You're also taking in a List, when you only need an IEnumerable - why restrict to a List when you could just as well work off any possible and not-yet-even-written implementation of the IEnumerable interface?

   Dim result As New List(Of Date)

This is another surprise. Given the signature, I'd expect result to be a String - but no, it's a List of ..dates?

   Return result.Min.ToString("HH:mm:ss")

Ah, I see what you're doing. So this loop is traversing the IEnumerable(Of Field) into a List(Of Field), so that you can leverage the IComparable implementation of Date and get the Min value in the list, and then return its string representation.

Let's see why we even need to explicitly iterate that IEnumerable in the first place:

    For Each item In fds
        Dim dt As Date = item.dateTime
        Dim s1 As TimeSpan = TimeSpan.Parse(dt.ToString("HH:mm:ss"))
        Dim s2 As TimeSpan = TimeSpan.Parse(item.sg)
        Dim s3 As TimeSpan = TimeSpan.Parse(item.os)

My lazy little brain doesn't like having to make associations/mappings between variables and their meaning. Whatever sg means, s2 should be named sg to mean the same thing. Same with s3, should be called os, and s1 would be timePortion or something like that, and then dt could be itemDate.

---

[♠] AVOID 2-letter identifiers [♠] _{(yes, in spades).}

Your naming conventions reflects your coding style, and that's not something you just "fix later" - you don't step into a codebase six months down the line and magically remember the context of every cryptic identifier. Write code as if the person maintaining it was a serial killer that knows where you live.

More seriously: you should name your identifiers while their meaning is crystal-clear in your brain, as soon as this meaning is crystalized, not "later". I cringe at fd1..fd2..fd3..fd4 in your throw-away code, seeing s1..s2..s3 in your actual code make me want to frantically rename everything just to get a freakin' idea of what stands for what.

Naming is important; writing code is hardly 20% of the job. The other 80% is spent reading code. Might as well make it readable.

---

Back to s1.

Dim s1 As TimeSpan = TimeSpan.Parse(dt.ToString("HH:mm:ss"))

This is... rather creative. Here's a less convoluted way:

Dim itemTime As TimeSpan = itemDate.TimeOfDay 's1

Then you have two nested conditions, with very similar branches - what's that smell?

result.Add(Date.Parse(itemDate.AddDays(1).ToShortDateString & " " & item.sg))
result.Add(Date.Parse(itemDate.AddDays(1).ToShortDateString & " " & item.os))
result.Add(Date.Parse(itemDate.ToShortDateString & " " & item.sg))
result.Add(Date.Parse(itemDate.ToShortDateString & " " & item.os))

Ok several things. Let's simplify a bit. A Date has an Add(TimeSpan) method that seems to have been written exactly for this:

result.Add(itemDate.AddDays(1).Date.Add(item.sg))
result.Add(itemDate.AddDays(1).Date.Add(item.os))
result.Add(itemDate.Date.Add(item.sg))
result.Add(itemDate.Date.Add(item.os))

Now here I'm assuming Field is a class that you own, too - and whose sg and os members (which should be Sg and Os... with an actual meaningful name for Heaven's sake) are of type TimeSpan. Your test code seems to indicate that they are of type String, which is a bad idea since it's really a TimeSpan - the database has it as a string? So what, your code isn't the database. You want your code to call a TimeSpan, a TimeSpan. Perform the parsing when you load the data from the record into your Field instance.

Otherwise you need to do this inside your loop, and you're wasting cycles parsing a TimeSpan from a String that should already be a TimeSpan - these two lines could be removed:

Dim sg As TimeSpan = TimeSpan.Parse(item.sg) 's2
Dim os As TimeSpan = TimeSpan.Parse(item.os) 's3

If you control the code for Field, change the type of sg and os to TimeSpan, and move the parsing earlier, outside of the GetMinTimeSpan function.

If Not item.sg = nullField Then

Right. item.sg is nullable. Don't make it a TimeSpan - make it a TimeSpan? (or Nullable(Of TimeSpan) and make your condition look like this instead:

If item.sg.HasValue Then

This is the result of all changes so far:

Private Function GetMinTimeSpan(fds As IEnumerable(Of Field)) As String
    Dim result As New List(Of Date)
    For Each item In fds
        Dim itemDate As Date = item.dateTime
        Dim itemTime As TimeSpan = itemDate.TimeOfDay

        If item.sg.HasValue Then
            If itemTime > item.sg Then
                result.Add(itemDate.AddDays(1).Date.Add(item.sg))
            Else
                result.Add(itemDate.Date.Add(item.sg))
            End If
        End If
        If itemTime > item.os Then
            result.Add(itemDate.AddDays(1).Date.Add(item.os))
        Else
            result.Add(itemDate.Date.Add(item.os))
        End If
    Next
    Return result.Min.ToString("HH:mm:ss")
End Function

---

Not so many changes, it's the same algorithm: you're iterating once (\$O(n)\$) to get every time stamp into result. Then result.Min iterates again to find the smallest value.

Why not just find the smallest value yourself, and ditch the List altogether?

Private Function GetMinTimeSpan(fds As IEnumerable(Of Field)) As String

    Dim currentMin As Date = DateTime.MaxValue

    For Each item In fds
        Dim itemDate As Date = item.dateTime
        Dim itemTime As TimeSpan = itemDate.TimeOfDay

        Dim temp As Date
        If item.sg.HasValue Then
            If itemTime > item.sg Then
                temp = itemDate.AddDays(1).Date.Add(item.sg)
                If temp < currentMin Then
                    currentMin = temp
                End If
            Else
                temp = itemDate.Date.Add(item.sg)
                If temp < currentMin Then
                    currentMin = temp
                End If
            End If
        End If
        If itemTime > item.os Then
            temp = itemDate.AddDays(1).Date.Add(item.os)
            If temp < currentMin Then
                currentMin = temp
            End If
        Else
            temp = itemDate.Date.Add(item.os)
            If temp < currentMin Then
                currentMin = temp
            End If
        End If
    Next
    Return currentMin.ToString("HH:mm:ss")
End Function

As I proceeded to write this, a doubt arised - have I broken functionality? I hope not! So many questions pop up all of a sudden:

• Why are you only returning the time portion of the smallest date in result?
• Why are you returning a String representation of that date?
• Why is the method called GetMinTimeSpan when it's actually returning a String that represents the time portion of a Date?
• Is this the desired behavior?

So I ran the code again (with the 10K iterations), and got this output:

00:00:00.0273893
00:00:00.0258447
00:00:00.0318047
00:00:00.0276503
00:00:00.0306695

For a single call:

00:00:00.0027795
00:00:00.0051286
00:00:00.0038952
00:00:00.0033619
00:00:00.0062803

Is it faster? Probably. Is it more efficient? Certainly is. Can it be further refactored? Sure! Is this answer long enough? Definitely!

Answer 3

I don't know how much of an optimization it is, but what really jumps out at me is all the conversions between TimeSpan, DateTime, and String. I'd classify it as "code smell" even if it's not bringing down the performance (though you really should run it through a profiler to be sure). I suspect it would be better to use the built-in methods of DateTime and TimeSpan in order to convert between them as directly as possible, without roundtripping through String.

I also don't understand why you build a List (Of Date). If you're going to compute a minimum, just designate a variable for it, and each time update it if necessary.