' Do you use comments in VB??? whats good and bad practice??

Morpheus

Hi all,

im programming in VB and i think i kinda go over the top with my comments, some of my betters find it a bit funny, but i find it easier on a monday morning to continue a project
when i know what i was last doing on a friday evening by reading the comments.

However there are times when it can look chaotic with green splurges all over the place. what are your methods and thoughts???

heres an example... no laughing please at my lack of programming savvy, im only new-ish at this....

copy it into a VB window to view it properly! and let me know what you think.



'***************************************************
' COMMAND: cmdCompare_Click()
'
' PARAMETERS:
' nSelected As Integer ' number of selected NAV's
' nNavFirst As Double ' value of first selection
' nNavSecond As Double ' value of second selection
' nNavDiff As Double ' value of NAV difference
' nRatio As Double ' value of ratio difference
' i As Integer ' Counter for looping thru listview contents
' blNavFirst As Boolean ' indicates whether a value for nNavFirst has been set
' blNavSecond As Boolean ' indicates whether a value for nNavSecond has been set
'
' RETURN VALUE:
' None
'
' DESCRIPTION:
' Calculates the difference between 2 user selected NAV's and outputs the
' difference and the percentage ratio to text boxes
'***************************************************
Private Sub cmdCompare_Click()

' PARAMETERS
Dim nSelected As Integer ' number of selected NAV's
Dim nNavFirst As Double ' value of first selection
Dim nNavSecond As Double ' value of second selection
Dim nNavDiff As Double ' value of NAV difference
Dim nRatio As Double ' value of ratio difference
Dim i As Integer ' Counter for looping thru listview contents
Dim blNavFirst As Boolean ' indicates whether a value for nNavFirst has been set
Dim blNavSecond As Boolean ' indicates whether a value for nNavSecond has been set

' Initialise parameters
nNavFirst = 0
nNavSecond = 0
nSelected = 0
blNavFirst = False
blNavSecond = False

' Loop through all items in the list view to find out
' what was selected
For i = 1 To Me.lstPriceListView.ListItems.Count

' check for selected items in the listview
If Me.lstPriceListView.ListItems(i).Checked = True Then
' increment number of selected items encountered
nSelected = nSelected + 1
' if the number selected is not greater than two

If Not nSelected > 2 Then
'if nNavFirst not assigned yet set its value

If blNavFirst = False Then
nNavFirst = Me.lstPriceListView.ListItems(i).SubItems(1)
blNavFirst = True

Else
' else set nNavSecond value
nNavSecond = Me.lstPriceListView.ListItems(i).SubItems(1)
blNavSecond = True
nRatio = Round(nNavFirst / nNavSecond * 100, 2)
nNavDiff = nNavFirst - nNavSecond
End If
End If
End If
Next

' make sure that not more than 2 records are selected
If nSelected > 2 Then
MsgBox "Please select 2 prices only", , "Selection Error"
' clear the checkboxes

For i = 1 To Me.lstPriceListView.ListItems.Count
Me.lstPriceListView.ListItems(i).Checked = False
Next

' re-initialise the boolean values
blNavFirst = False
blNavSecond = False
'exit the sub procedure
Exit Sub
End If

' display results on user interface
txtPriceRatio = nRatio & "%"
txtPriceDiff = Format(nNavDiff, "##,##0.00")

' reset all selected items to unselected for next use
For i = 1 To Me.lstPriceListView.ListItems.Count
Me.lstPriceListView.ListItems.Item(i).Checked = False
Next

' reset counter and clear listview
nSelected = 0
Me.lstPriceListView.ListItems.Clear

End Sub

:rolleyes:

cheers

boo-boo

I think thats fine , hell of a lot better than no comments.
You should use indentation for conditional code though, ie

If (myVar = true) then
x = 1
Else
x = 2
End If


or

Do while x < 10
x=x+1

Loop



its far easier to read. If you'll be doing lots of VB stuff check out the style guide at the MS site.

regi

dim colour ' as a revolutionary new way to wash your hair!

Talliesin

Originally posted by Regi
dim colour ' as a revolutionary new way to wash your hair!

When I came to take over that code from Regi it took a good half-hour to find out what the variable "colour" was doing.

It turned out it was doing nothing, and was completely unreferenced…

Morpheus

thanks for the tips,

actually the code was indented, but when i pasted it in, it became un-indented

mr_angry

I think the existing comments are perfectly alright. If anything, I'd say I usually have more!

Everybody is entitled to their own coding and commentary style, but the readability and maintainability of the code is directly proportional to the amount of comments in a lot of cases.

The more the merrier I say!

RicardoSmith

I tend to use a lot of comments, so I would say thats fine.

Talliesin

A few things I'd change:

1. Don't call the local variables parameters, people might think they are parameters.

2. Delete the dead code. For example replace:

	[COLOR=green]' re-initialise the boolean values[/color]
	blNavFirst = False
	blNavSecond = False
	[COLOR=green]'exit the sub procedure[/color]
	Exit Sub
End If

With :

	[COLOR=green]'exit the sub procedure[/color]
	Exit Sub
End If

As setting those local variables to false has no effect (except decreasing performance if the optimiser doesn't remove them) since they are deleted when you exit the procedure.

If dead code helps readability in some way, or relates to a possible adaptation of the code (i.e. it isn't dead code in a likely variant on the procedure) I'd say leave it in, otherwise kill it as it's just making the procedure look more complicated than it is.

For example, your initialisation code is dead code, since VB always initialises Integers and Doubles to 0 and Booleans to False, but it is good to have the booleans initialised, since it makes things clearer. The intial values of the doubles aren't used so initialising them is a potential source of confusion.

3. Commenting code isn't just about the comments, it's also about making the code as self-documenting as possible by using good variable names, strong typing, good procedure names, breaking long procedures into several procedures in a coherent way and favouring the conventional over the unusual.

Sometimes this adds to what you see in the comments (indeed you should see the comments as adding to the inherent readability of the code itself), sometimes it makes some comments redundant, for instance:

	[COLOR=green]'exit the sub procedure[/color]
	Exit Sub

What else is Exit Sub going to do except for exitting the subprocedure? If you genuinely feel that that comment adds something then leave it in, otherwise take it out and it will make your othe code and comments stand out more.

3b. Reconsider your variable naming, you are using n as a prefix to both integers and doubles. These are variable types with different semantics, sizes and conversion rules. If you are going to use a prefix then it should be one that reflects this. It's particularly important with the built-in types because an attempt to assign one to the other won't be noticed by the compiler (VB has no warnings, something is either valid or it isn't) and hence disasterous casts could be easily made and hard to track.

4. Use common conventions whenever you can. For example (with reference to point 3b), there's no one true Hungarian, and there is especially a lot of variation with the built-in types. However documentation often makes a suggestion for prefixes for controls, and it's best to stick to them. A lot of people would think that lstPriceListView was a listbox, not a listview. While the suggested prefixes for built-in types never caught on (they are a bit ungainly for such frequently used variables, many of the suggested prefixes for objects should be used if practical.

5. In VB you have to find a balance with the number of calls you make into other procedures if the code is time-critical, because it isn't great at optimising those calls away. In general though a function that decides between two different complicated pieces of logic based on some criteria (or even if it includes one really complicated piece of logic for every call) it's a good idea to put that logic into another procedure.

6. With event handlers consider whether you are likely to want what it does performed called programatically. If so while it is possible to directly call the handler (indeed you can even make it Public and call it from another class) it is better to create another procedure and then have it called from both the event handler and the other piece(s) of code which call it. If you are unsure whether you are likely to want to call such a procedure in the future assume that you will.

7. Use the built-in help mechanisms well. Don't document much about what is documented in VBs own documentation. Put in-depth details about the use (but not implementation) of public procedures into the projects help file and give them a help context. Give them concise but useful helpstrings for object browser to use. You should not consider any major component finished until this is done.

8. Favour object orientated over procedural, favour For...Next over numeric iteration (this tends to be faster as well as more readable), cache results rather than repeatedly call the same procedure (again this increases speed as well as readability). ALWAYS favour the languages error-handling mechanisms over ad-hoc mechanisms. Use "Me" when it gives you something, avoid it when it's just something else for the next guy to read and mentally process.

9. Use explicit casts even when implicit casts will work, in the following:

nNavFirst = Me.lstPriceListView.ListItems(i).SubItems(1)

It isn't clear that a conversion is going on. Casts and Conversions are evil, and their occurance should be noted, using the explicit function is a good way of doing this (all VB coders will immediately know what's going on):

nNavFirst = CDbl(Me.lstPriceListView.ListItems(i).SubItems(1))

10. Avoid magic strings and magic numbers. Use constants with clear names.

11. Conventions and opinions differ about comment blocks before procedures. Generally I like to put stuff there that applies from outside of the procedure, its parameters, purpose, important global or module-level dependencies, errors likely to be raised.

Local variables and other implementation-specific stuff I prefer to document inside of the procedure, this separates documentation on using the procedure from documentation on how it actually works. I don't want to think about how it works once its written and tested (any procedure that requires you to think about how it works to use it is badly-designed, so there is no overlap between those two cases).

Talliesin

A very quick re-write following my suggestions. This might be buggy as hell, and there are some obvious ommisions (use of non-localised messages for one thing).

[COLOR=green]'Errors raised and caught local to this module[/COLOR]
Private Enum LOCALERRORS
    lcerrWrongNumberNavs = vbObjectError + 701 [COLOR=green]'    Number selected wasn't 2.[/COLOR]
End Enum

Private Const DIFF_FORMAT_STRING As String = "##,##0.00"

[COLOR=green]'***************************************************
' CompareNav()
'
' Scan lvwPriceListView for 2 selected NAVs.
' Calculate difference and ratio. Display these in
' txtPriceDiff and txtPriceRatio.
'
' Errors:
' Raises lcerrWrongNumberNavs if there are less or
' more than 2 NAVs selected.
'
' Dependencies:
' lvwPriceListView - ListView
' txtPriceDiff - TextBox
' txtPriceRatio - TextBox
' lcerrWrongNumberNavs - Error Constant
'***************************************************[/COLOR]
Private Sub CompareNav()
    Dim dNavFirst As Double         [COLOR=green]' value of first selection.[/COLOR]
    Dim dNavSecond As Double        [COLOR=green]' value of second selection.[/COLOR]
    Dim dNavDiff As Double          [COLOR=green]' difference between the two Navs.[/COLOR]
    Dim dRatio As Double            [COLOR=green]' ratio difference of the two Navs.[/COLOR]
    
    Dim bFirstNavFound As Boolean   [COLOR=green]' Set once we've obtained dNavFirst.[/COLOR]
    Dim bSecondNavFound As Boolean  [COLOR=green]' Set once we've obtained dNavSecond.[/COLOR]
    
    Dim liIterate As ListItem       [COLOR=green]' Used to examine each ListItem[/COLOR]
    
    [COLOR=green]' Initialise local variables[/COLOR]
    bFirstNavFound = False
    bSecondNavFound = False
    
    [COLOR=green]' Loop through all items in the list view to find out
    ' what was selected[/COLOR]
    For Each liIterate In lvwPriceListView.ListItems
    
        [COLOR=green]' check for selected items in the listview[/COLOR]
        If liIterate.Checked Then
            
            If bSecondNavFound Then
                [COLOR=green]'We already have our 2 values, so this is an error condition.[/COLOR]
                Err.Raise lcerrWrongNumberNavs
            End If
            
             [COLOR=green]'if dNavFirst not assigned yet set its value[/COLOR]
            
             If Not bFirstNavFound Then
                 dNavFirst = CDbl(liIterate.SubItems(1))
                 bFirstNavFound = True
             Else
                 [COLOR=green]' else set dNavSecond value[/COLOR]
                 dNavSecond = CDbl(liIterate.SubItems(1))
                 bSecondNavFound = True
             End If
        
        End If
    Next
    
    [COLOR=green]' make sure that we have our two numbers.[/COLOR]
    If Not bSecondNavFound Then
        Err.Raise lcerrWrongNumberNavs
    End If
    
    dRatio = Round(dNavFirst / dNavSecond * 100, 2)
    dNavDiff = dNavFirst - dNavSecond
    
    [COLOR=green]' display results on user interface[/COLOR]
    txtPriceRatio = dRatio & "%"
    txtPriceDiff = Format(dNavDiff, DIFF_FORMAT_STRING)
    
End Sub

Private Sub cmdCompare_Click()
    [COLOR=green]'Compare Navs and then clear the listview.
    'If Nav Comparison fails alert the user, but leave the listview so they
    'can rectify and repeat the request.[/COLOR]

    On Error GoTo ErrHandle
    CompareNav
    lvwPriceListView.ListItems.Clear
ErrHandle:
    With Err
        Select Case .Number
            Case 0
                [COLOR=green]'No Error[/COLOR]
            Case lcerrWrongNumberNavs
                MsgBox "Please select 2 prices only", vbExclamation, "Selection Error"
                .Clear
            Case Else
                [COLOR=green]'Unexpected Error - We could have a general MsgBox call,
                'or we could pass the error on. Eventually (if not caught)
                'it will stop the program, and debugging can examine it.[/COLOR]
                .Raise .Number, "SomeProject.SomeModule", .Description, .HelpFile, .HelpContext
        End Select
    End With
End Sub