Better solution, timer, stopwatch, timespan

Question

I am working on small tool for tracking duration of various activities.

In this example we have 3 activities, Drive, Walk and Wait.

Each activitiy is a button on Form1

Example:

• Click on button Drive, stopwatch "SW" and timer "Tmr" are started and counting "Drive" time.
• After 5 seconds I click on button Wait, SW and Tmr are stopped, SW1 and Tmr1 are started and counting time for "Wait" activity.
• Click again on button Drive, SW1 and Tmr1 as stopped, SW and Tmr started and time is resumed from 5th second

And so on, can be one or more activities included. At the end of measuring I have total duration for each activity.

This Code below is actually working well. Function is called from the Form1, measuring is started and later I have values in public variables available.

Module:

Dim SW, SW1, SW2 As New Stopwatch
Dim WithEvents Tmr, Tmr1, Tmr2 As New Timer
Dim stws() = {SW, SW1, SW2}
Dim tmrs() = {Tmr, Tmr1, Tmr2}
Public Drive, Walk, Wait As String

Public Function WhichButton(btn As Button)

WhichButton = btn.Text

        Select Case WhichButton
            Case "Drive"
                For Each s As Stopwatch In stws
                    s.Stop()
                Next
                For Each t As Timer In tmrs
                    t.Stop()
                Next
                SW.Start()
                Tmr.Start()
            Case "Wait"
                For Each s As Stopwatch In stws
                    s.Stop()
                Next
                For Each t As Timer In tmrs
                    t.Stop()
                Next
                SW.Start()
                Tmr1.Start()
            Case "Walk"
                For Each s As Stopwatch In stws
                    s.Stop()
                Next
                For Each t As Timer In tmrs
                    t.Stop()
                Next
                SW2.Start()
                Tmr2.Start()
        End Select
End Function

Private Sub Tmr_Tick(sender As Object, e As System.EventArgs) Handles Tmr.Tick
    Dim elapsed As TimeSpan = SW.Elapsed
    Drive = $"{elapsed.Hours:00}:{elapsed.Minutes:00}.{elapsed.Seconds:00}"
End Sub

Private Sub Tmr1_Tick(sender As Object, e As System.EventArgs) Handles Tmr1.Tick
    Dim elapsed As TimeSpan = SW1.Elapsed
    Walk = $"{elapsed.Hours:00}:{elapsed.Minutes:00}.{elapsed.Seconds:00}"
End Sub

Private Sub Tmr2_Tick(sender As Object, e As System.EventArgs) Handles Tmr2.Tick
    Dim elapsed As TimeSpan = SW2.Elapsed
    Wait = $"{elapsed.Hours:00}:{elapsed.Minutes:00}.{elapsed.Seconds:00}"
End Sub

Reason im here is because I'm not happy with this solution and I don't have a knoweledge for advanced one. The probem here is that I can have X number of Buttons, can add new or remove few, it depends on situation, and I don't want to write block of Code for each. Also if I Change a text property of the button, Select Case will not work. So I want to create timers and stopwatches dynamically for each button.

I would like to start with this:

 Dim timers As List(Of Timer) = New List(Of Timer)

Private Sub Form1_Load(sender As Object, e As EventArgs) Handles MyBase.Load

    For Each btn As Button In Panel1.Controls.OfType(Of Button)
        timers.Add(New Timer() With {.Tag = btn.Name})
        AddHandler btn.Click, AddressOf Something
    Next

End Sub

Public Sub Something(sender As Object, e As EventArgs)
    Dim btn = DirectCast(sender, Button)

    Dim tmr As Timer = timers.SingleOrDefault(Function(t) t.Tag IsNot Nothing AndAlso t.Tag.ToString = btn.Name)

End Sub

Here I can refer to Timer over the Tag property but I have no idea how to implement stopwatch and timespan. Thanks for reading and any help is appreciated, suggestions, pseudocode, code examples.

Answer 1

Firstly, there's no point using three Timers. A single Timer can handle all three times. Secondly, based on what you've posted, there's no point using any Timer. The only reason I could see that a Timer would be useful would be to display the current elapsed time in the UI constantly, but you're not doing that. Repeatedly setting those String variables is pointless if you're not going to display them. Just get the Elapsed value from the appropriate Stopwatch if and when you need it.

As for your Buttons' Click event handler, it's terrible too. The whole point of a common event handler is because you want to do the same thing for each object so you only have to write the code once. If you end up writing separate code for each object in that common event handler then that defeats the point and makes your code more complex instead of less. You should be using separate event handlers for each Button.

If you were going to go with a common event handler though, at least extact out the common code. You have the same two For Each loops in all three Case blocks. That should be done before the Select Case and then only start the appropriate Stopwatch in each Case.

I don't think that you should be using Buttons though. You should actually be using RadioButtons. You can set their Appearance property to Button and then they look just like regular Buttons but still behave like RadioButtons. When you click one, it retains the depressed appearnce to indicate that it is checked and clicking a different one will release the previously-depressed one. In that case, your code might look like this:

Private ReadOnly driveStopwatch As New Stopwatch
Private ReadOnly waitStopwatch As New Stopwatch
Private ReadOnly walkStopwatch As New Stopwatch

Private Sub driveRadioButton_CheckedChanged(sender As Object, e As EventArgs) Handles driveRadioButton.CheckedChanged
    If driveRadioButton.Checked Then
        driveStopwatch.Start()
    Else
        driveStopwatch.Stop()
    End If
End Sub

Private Sub waitRadioButton_CheckedChanged(sender As Object, e As EventArgs) Handles waitRadioButton.CheckedChanged
    If waitRadioButton.Checked Then
        waitStopwatch.Start()
    Else
        waitStopwatch.Stop()
    End If
End Sub

Private Sub walkRadioButton_CheckedChanged(sender As Object, e As EventArgs) Handles walkRadioButton.CheckedChanged
    If walkRadioButton.Checked Then
        walkStopwatch.Start()
    Else
        walkStopwatch.Stop()
    End If
End Sub

Because checking a RadioButton automatically unchecks any other, each CheckedChanged event handler only has to worry about its own Stopwatch.

If you wanted to display the elapsed time for a particular Stopwatch when it stops, you do that when it stops, e.g.

Private Sub driveRadioButton_CheckedChanged(sender As Object, e As EventArgs) Handles driveRadioButton.CheckedChanged
    If driveRadioButton.Checked Then
        driveStopwatch.Start()
    Else
        driveStopwatch.Stop()
        driveLabel.Text = driveStopwatch.Elapsed.ToString("hh\:mm\:ss")
    End If
End Sub

That overload of TimeSpan.ToString was first available in .NET 4.5 I think, so you should use it unless you're targeting .NET 4.0 or earlier.

If you did want to display the current elapsed time constantly then, as I said, you only need one Timer. You would just let it run all the time and update appropriately based on the Stopwatch that is currently running, e.g.

Private Sub displayTimer_Tick(sender As Object, e As EventArgs) Handles displayTimer.Tick
    If driveStopwatch.IsRunning Then
        driveLabel.Text = driveStopwatch.Elapsed.ToString("hh\:mm\:ss")
    ElseIf waitStopwatch.IsRunning Then
        waitLabel.Text = waitStopwatch.Elapsed.ToString("hh\:mm\:ss")
    ElseIf walkStopwatch.IsRunning Then
        walkLabel.Text = walkStopwatch.Elapsed.ToString("hh\:mm\:ss")
    End If
End Sub

You haven't shown us how you're displaying the elapsed time so that's a bit of a guess. In this scvenario, you should definitely still update the Label when a Stopwatch stops, because the Timer won't update that Label on the next Tick.

You would presumably want a Button somewhere that could stop and/or reset all three Stopwatches. That would mean setting Checked to False on all three RadioButtons and then calling Reset on all three Stopwatches. You'll probably want to clear/reset the Labels too.

There's also a potential gotcha using RadioButtons like this. If one of your RadioButtons is first in the Tab order then it will recieve focus by default when you load the form. Focusing a RadioButton will check it, so that would mean that you'd start a Stopwatch by default. If that's not what you want, make sure that some other control is first in the Tab order. If you can't do that for some reason, handle the Shown event of the form, set ActiveControl to Nothing, uncheck that RadioButton and reset the corresponding Stopwatch and Label.

As a final, general message, notice that I have named everything so that even someone with no prior knowledge of the project would have no doubt what everything was and what it was for. Names like SW, SW1 and SW2 are bad. Even if you realised that SW meant Stopwatch, you have no idea what each one is actually for. In this day of Intellisense, it's just lazy use names like that. Every experienced developer can tell you a story about going back to read their own code some time later and having no idea what they meant by various things. Don't fall into that trap and make sure that you get into good habits early.

EDIT:

As a bonus, here's a way that you can use a common event handler properly. Firstly, define a custom Stopwatch class that has an associated Label:

Public Class StopwatchEx
    Inherits Stopwatch

    Public Property Label As Label

End Class

Once you make that association, you automatically know which Label to use to display the elapsed time for a Stopwatch. Next, define a custom RadioButton class that has an associated Stopwatch:

Public Class RadioButtonEx
    Inherits RadioButton

    Public Property Stopwatch As StopwatchEx

End Class

Next, use that custom class on your form instead of standard RadioButtons. You can add them directly from the Toolbox (your custom control will be added automatically after building your project) or you can edit the designer code file and change the type of your controls in code. There is a certain amount of risk in the latter option so be sure to create a backup beforehand. Once that's all done, change the type of your Stopwatches and handle the Load event of the form to create the associations:

Private ReadOnly driveStopwatch As New StopwatchEx
Private ReadOnly waitStopwatch As New StopwatchEx
Private ReadOnly walkStopwatch As New StopwatchEx

Private Sub Form1_Load(sender As Object, e As EventArgs) Handles MyBase.Load
    'Associate Stopwatches with RadioButtons
    driveRadioButton.Stopwatch = driveStopwatch
    waitRadioButton.Stopwatch = waitStopwatch
    walkRadioButton.Stopwatch = walkStopwatch

    'Associate Labels with Stopwatches
    driveStopwatch.Label = driveLabel
    waitStopwatch.Label = waitLabel
    walkStopwatch.Label = walkLabel
End Sub

You can now use a single method to handle the CheckedChanged event for all three RadioButtons because you can now do the exact same thing for all three of them:

Private Sub RadioButtons_CheckedChanged(sender As Object, e As EventArgs) Handles driveRadioButton.CheckedChanged,
                                                                                  waitRadioButton.CheckedChanged,
                                                                                  walkRadioButton.CheckedChanged
    Dim rb = DirectCast(sender, RadioButtonEx)
    Dim sw = rb.Stopwatch

    If rb.Checked Then
        sw.Start()
    Else
        sw.Stop()
        sw.Label.Text = sw.Elapsed.ToString("hh\:mm\:ss")
    End If
End Sub

The RadioButton that raised the event tells you which Stopwatch to use and that tells you which Label to use, so there's no need to write different code for each one.

The Tick event handler of the Timer can also treate each Stopwatch with common code:

Private Sub displayTimer_Tick(sender As Object, e As EventArgs) Handles displayTimer.Tick
    For Each sw In {driveStopwatch, waitStopwatch, walkStopwatch}
        If sw.IsRunning Then
            sw.Label.Text = sw.Elapsed.ToString("hh\:mm\:ss")
            Exit For
        End If
    Next
End Sub

You can create the array atthe class level but, as it's only being used in this one place, it makes sense to create it here. The performance hit is insignificant and it makes the code more readable by creating things where they are used.

Note that I did use abbreviations for variable names in this code. That's for two reasons. Firstly, they are variables that will refer to different objects at different times. That means that using a name specific to the purpose of the object is not possible. You could use a context-based name, e.g. currentRadioButton, but I don't do that here because of the second reason.

That second reason is that they are local variables used in a very limited scope. The rb and sw variables are not used more than a few lines from where they are declared so it's hard to not understand what they are. If you name a field like that then, when you see it in code, you have to look elsewhere to find out what it is. In this code, if you're looking at a usage of one of those variables then the declaration is in eyeshot too, so you'd have to be blind not to see what type you're dealing with. Basically, if a variable is used a long way from its declaration then I suggest a meaningful, descriptive name. If it is only used within a few lines of its declaration though, a brief name is OK. I generally tend to use the initials of the type, as I have done here. If you need multiple local variables of that type, I generally prefer to use descriptive names to disambiguate them rather than using numbers. Sometimes, though, there's really no purpose-specific way to do that, in which case numbers are OK, e.g. comparing two Strings without context might use s1 and s2 as variable names.