Recommendations to improve my code

[Quinncunx]

Hi, I was wondering if anybody who is a more experienced VB developer than I would mind casting an eye over my source code and offering any advice on how it could be improved.

The application is timer that runs for 7 minutes, the first minute is phase 1 and is considered rest time where there are no changes to be seen, phase 2 lasts 6 mins and there is a second and minute counter counting up, bells chime at 1, 6 and 7 mins (though visually this is at 0, 5 and 6 mins)

I still need to add all the user controls in such as stop pause and resume, I was wondering if there is a neat way of doing that as in could I put them in another module? to avoid it looking like this (taken from my first attempt at this timer)

'start button console
    Private Sub Button1_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles bStart.Click
       ToolStripLabel2.Enabled = False
        bStart.Enabled = False
        restwindow.Show()
        If CBLoop.Checked Then
            BGTimer.Start()
        End If
        RestTimer.Start()
        bPause.Hide()
        bResume.Hide()
        bStop.Enabled = True
        StopToolStripMenuItem.Enabled = True
        PauseToolStripMenuItem.Enabled = True
        ResumeToolStripMenuItem.Enabled = False
        StartToolStripMenuItem.Enabled = False
        bPause.Enabled = True
End Sub

here is the code from the current version

Public Class MainWindow

    Dim t0, t1, t2, t3, t4, i1, isecs, m1, m2, m3, m4, t5 As Integer

    Private Sub MainWindow_Load(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles MyBase.Load
        t0 = 0
        i1 = 1
        t5 = 6
        t1 = 60
        t2 = t1 * 6
        t3 = t2 + t1
        m1 = t1 * 2
        m2 = m1 + t1
        m3 = m1 * 2
        m4 = m3 + t1
        isecs = isecs + i1
        tbRest.Text = t0
        tbSecs.Text = t0
        tbMins.Text = t0

    End Sub
    Private Sub Timer_Tick(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles Timer.Tick
        tbRest.Text = tbRest.Text + i1
        If tbRest.Text > t1 Then
            tbSecs.Text = tbSecs.Text + 1
        End If
    End Sub

    Private Sub tbSecs_TextChanged(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles tbSecs.TextChanged
        If tbSecs.Text = t1 And Timer.Enabled = True Then
            tbSecs.Text = t0
        End If
    End Sub
    Private Sub tbRest_TextChanged(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles tbRest.TextChanged
        If tbRest.Text = t1 Then
            My.Computer.Audio.Play(My.Resources.bell, AudioPlayMode.Background)
        ElseIf tbRest.Text = m1 Then
            tbMins.Text = tbMins.Text + i1
        ElseIf tbRest.Text = m2 Then
            tbMins.Text = tbMins.Text + i1
        ElseIf tbRest.Text = m3 Then
            tbMins.Text = tbMins.Text + i1
        ElseIf tbRest.Text = m4 Then
            tbMins.Text = tbMins.Text + i1
        ElseIf tbRest.Text = t2 Then
            My.Computer.Audio.Play(My.Resources.bell, AudioPlayMode.Background)
            tbMins.Text = tbMins.Text + i1
        ElseIf tbRest.Text = t3 Then
            My.Computer.Audio.Play(My.Resources.bell, AudioPlayMode.Background)
            tbMins.Text = tbMins.Text + i1
        End If
    End Sub
    Private Sub tbMins_TextChanged(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles tbMins.TextChanged
        If CheckBox1.Checked = True And tbMins.Text = t5 Then
            tbMins.Text = t0
            tbSecs.Text = t0
            tbRest.Text = t0 
        ElseIf tbMins.Text = t5 Then
            Timer.Stop()
        End If
    End Sub
    Private Sub Button1_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles Button1.Click
        Timer.Start()
    End Sub
End Class

[Reverend Jim]

When you ask a question like that you are going to get as many different opinions as you have people responding. Everyone has their own style of coding and even that style can evolve. See Not Invented Here for an example. Please keep in mind that the following reflects my style (and opinion) only. It is not my intention to trash your code or your style. We all start at a certain level and we all (I hope) strive to improve. There are far better programmers out there than I am so there is much room for improvement in my code as well.

But you asked for comments so here goes. I'll start with the aesthetic because I believe that code should be pleasing to the eye and easy to read as well as functional.

Put a header at the top of your main module. My headers look like this:

'
'  Name:
'
'    appname
'
'  Description:
'
'    A short description of what the program does.
'
'  Usage:
'
'    If the usage is not obvious this should give some details. If it is a console
'    app then this section should explain what the parameters are.
'
'  Notes:
'
'    If the program requires any addition components, or particular versions of
'    components then this should be detailed here.
'
'  Audit:
'
'    2012-02-12  my name - original code (provide summary of changes with dates)
'

Use white space (blank lines) to separate your code into blocks. When writing prose, writers separate blocks of related ideas into paragraphs. This makes the prose flow more naturally and makes it easier to read. In technical writing, the first sentence in a paragraph acts as an introduction to the idea being explored in that paragraph. Similarly, a block of code should usually have a comment indicating what that block does.

A comment should explain what, and possibly how and why (if necessary). A useful comment is "update the counter of how many objects are allocated". A useless comment is "add 1 to x".

Each function and sub should have a mini-header.

All controls (other than static labels) should have a name that describes the purpose of that control. "Button1" is a bad name. "btnGenStats" is a better name because the function is alluded to by the name, and someone reading the code can tell that the code refers to a button (btn prefix) control. Also, if you have many buttons (for example) with names like Button1, Button2, etc, you are more likely to make a mistake (change the code for the wrong button).

Variable names should be descriptive, especially as the scope increases. A variable that has limited scope (such as a loop index) can be short. For example

For i As Integer = 0 to 20
    .
    .
    .
Next

"i" has scope only for the duration of the loop. Avoid variables like m1, m2, m3, etc. This says to the world "I am an engineer". Variables should be declared one per line with a short comment on that same line as in:

Private Authors As TreeNode     'all authors are added under this node                  
Private Library As String       'the fully qualified name of the active library         
Private LogFile As String       'file for audit trail of all library operations

Use Select clauses where appropriate. For example

If tbRest.Text = t1 Then
    My.Computer.Audio.Play(My.Resources.bell, AudioPlayMode.Background)
ElseIf tbRest.Text = m1 Then
    tbMins.Text = tbMins.Text + i1
ElseIf tbRest.Text = m2 Then
    tbMins.Text = tbMins.Text + i1
ElseIf tbRest.Text = m3 Then
    tbMins.Text = tbMins.Text + i1
ElseIf tbRest.Text = m4 Then
    tbMins.Text = tbMins.Text + i1
ElseIf tbRest.Text = t2 Then
    My.Computer.Audio.Play(My.Resources.bell, AudioPlayMode.Background)
    tbMins.Text = tbMins.Text + i1
ElseIf tbRest.Text = t3 Then
    My.Computer.Audio.Play(My.Resources.bell, AudioPlayMode.Background)
    tbMins.Text = tbMins.Text + i1
End If

should be rewritten as

Select Case tbRest.Text 
    Case t1
        My.Computer.Audio.Play(My.Resources.bell, AudioPlayMode.Background)
    Case m1
        tbMins.Text = tbMins.Text + i1
    Case m2
        tbMins.Text = tbMins.Text + i1
    Case m3
        tbMins.Text = tbMins.Text + i1
    Case m4
        tbMins.Text = tbMins.Text + i1
    Case t2 
        My.Computer.Audio.Play(My.Resources.bell, AudioPlayMode.Background)
        tbMins.Text = tbMins.Text + i1
    Case t3
        My.Computer.Audio.Play(My.Resources.bell, AudioPlayMode.Background)
        tbMins.Text = tbMins.Text + i1
End Select

And because you have used very non-descriptive variable names like m1, etc. you should put an inline comment on each case statement to explain that case. Incidentally, none of your sections (in the first or second version) will ever be executed because you are comparing a string (tbRest.Text) to integers.

How's that for starters? Discouraged yet? (don't be)

Incidentally, I have my IDE set so that comments are displayed with black letters on a silver-grey background. This makes the comments stand out. With properly written comments it is easier to read just the comments and ignore the code (until later). And, if you comment out a large block of code it is really obvious. I once had to determine the function of a twenty page block of FORTRAN code. It would have been easier had I noticed that pages 4-12 had been commented out with a block comment. If my editor (back in 1985) had supported syntax colouring it would have been obvious.

[Quinncunx]

Thank you ever so much for you comments, they're both helpful and a great learning experience for me. I shall certainly be implementing some of your comments, especially the aesthetic comments, I will also use the Select Clauses as recommended. It is a great help thank you.

Incidentally, none of your sections (in the first or second version) will ever be executed because you are comparing a string (tbRest.Text) to integers.

I have been told this by quite a few people, but the program seems to run as I would expect it too I have tried various other ways of coding it (which people have recommended) but they have all failed, is there something you can suggest?

Thank you again.

[Reverend Jim]

Compare like to like as in

Select Case CInt(tbRest.Text )
    Case 1
        .
    Case 30
        .
etc

Of course, that assumes (something your code shouldn't do) that tbRest.Text can be converted to an integer value.

If there is one thing I have learned it is that no matter how I do something there is almost always a better way. At some point (especially when you have a deadline) you have to abandon the quest for best and settle for good enough.

[Reverend Jim]

By the way (and I can't believe I didn't notice this earlier), the code that I rewrote as a select clause could be simplified to

Select Case CInt(tbRest.Text)
    Case t1
        My.Computer.Audio.Play(My.Resources.bell, AudioPlayMode.Background)
    Case m1, m2, m3, m4
        tbMins.Text = tbMins.Text + i1
    Case t2, t3
        My.Computer.Audio.Play(My.Resources.bell, AudioPlayMode.Background)
        tbMins.Text = tbMins.Text + i1
End Select

[Quinncunx]

Thank you again for your feedback, apologies for a delayed reply, I was tied up with school work. With regards to this

Compare like to like as in

Select Case CInt(tbRest.Text )
    Case 1
        .
    Case 30
        .

I am not entirely sure I understand what you mean. In respect to the last post you made, I had done that when I converted my code to that recommendation.

Again, thank you for your continued patience and advice, it is very much appreciated.

[Reverend Jim]

When doing comparisons you want to compare like to like. You were comparing text to integer. For example, the statement

If "3" = 3 Then

would fail because the string value "3" is not the same as the number 3. Even if you stored the "3" as a character instead of a string the comparison would still fail because "3" (at least in ASCII) has the internal value of 51.

[jlego]

I recommend picking a copy up of the book 'The Programatic Programmer'.
It isn't language specific but a general guidance to best practices whenever it comes to developing.