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!
StopWatch
is running when you are declaring the variables in your sample - Is this intended? \$\endgroup\$