Bob Phillips sent me this magnificent example of quality code commenting (from MS somewhere, apparently)
Sub SelectEveryNthRow()
' Initialize ColsSelection equal to the number of columns in the
' selection.
ColsSelection = Selection.Columns.Count
' Initialize RowsSelection equal to the number of rows in your
' selection.
RowsSelection = Selection.Rows.Count
' Initialize RowsBetween equal to three.
RowsBetween = 3
' Initialize Diff equal to one row less than the first row number of
' the selection.
Diff = Selection.Row - 1
' Resize the selection to be 1 column wide and the same number of
' rows long as the initial selection.
Selection.Resize(RowsSelection, 1).Select
' Resize the selection to be every third row and the same number of
' columns wide as the original selection.
Set FinalRange = Selection. _
Offset(RowsBetween - 1, 0).Resize(1, ColsSelection)
' Loop through each cell in the selection.
For Each xCell In Selection
' If the row number is a multiple of 3, then . . .
If xCell.Row Mod RowsBetween = Diff Then
' ...reset FinalRange to include the union of the current
' FinalRange and the same number of columns.
Set FinalRange = Application.Union _
(FinalRange, xCell.Resize(1, ColsSelection))
' End check.
End If
' Iterate loop.
Next xCell
' Select the requested cells in the range.
FinalRange.Select
End Sub
Stunning!
Thanks Bob !
As Bob says, if ever there was an argument against commenting, this is it (Bob inserted the blank lines to make it readable)
It might be a bit arrogant, but I think you need to write your code on the understanding whoever comes later will have a reasonable level of competence. If they can’t work out what
Selection.Columns.Count
means then I’m not sure they should be poking around VBA. (or anything important really).
What do you think?
cheers
Simon
Thursday, 17th July, 2008 at 4:12 pm |
“If it was hard to write, it should be hard to read.” – Chip Pearson
Thursday, 17th July, 2008 at 5:12 pm |
Expanding on the quote that jonpeltier cited from Chip Pearson, Programming a piece of software is not a linear process, unless you are building a simple, less-than-20-line, program. You are constantly debugging and adding, deleting, and changing code. Even if I never intend for anyone else to read my code, I still comment the heck out of it. Sometimes, I’ll even comment in what I was thinking, or how I came to the conclusion to do it this certain way, rather than an alternative method. I’ll comment out old processes and comment why I removed it, or if I plan to add it in later. This way, when I come back to a year later, I can pick up where I left off without rethinking a lot of the processes. And then on the other hand, if someone else wants to poke around, they won’t be helplessly lost. :D
Thursday, 17th July, 2008 at 5:50 pm |
I think there is a distinction between documenting the intent or purpose of your code such as the method behind your madness (the reason for taking a particular approach), assumptions, dependencies and so on, compared against repeating what each line of code does in your commentary.
As Bryan noted, he comments how he decided to do it a certain way. I’ll even include URL’s to MSDN articles or sample code which impacted the design.
This example appears to be more of a tutorial. But in true MS style, there’s no variable declarations or naming conventions.
Thursday, 17th July, 2008 at 6:46 pm |
For basic procedures who needs naming conventions?
I use naming the conventions from K&R C or the last numerical programming text I read. I’m still waiting to find a rational as opposed to religiously orthodox reason to use variables named like lArrayIndex1, lArrayIndex2 and lArrayIndex3 rather than i, j and k.
This one’s the most offensive comment:
‘ If the row number is a multiple of 3, then . . .
If xCell.Row Mod RowsBetween = Diff Then
Go to all that trouble to abstract the number of rows in RowsBetween then put the magic number into the comment. Classic!
Thursday, 17th July, 2008 at 7:20 pm |
The best part of that code is that it only works if you start the selection in row 1, any other row and it fails in its objectives. Commented to death, crap code.
I posted this to Simon because I know his views are the extreme of what I think, that most comments are a waste of time. If ever I pick up someone else’s code I tend to strip out the comments because I can then read the code better, and experience tells me that the comments will enlighten me very little, if at all. As to my own code, yes my comments are great of course, at the time I wrote them. Going back 6 months, a year later, I am just as confused by them as I am by others … my techniques will have changed, my naming conventions, all manner of things. Far better to get cracking on the code IMO.
And worst of all … module and procedure banners. I did a job for a major high street retailier last year and I ad to include this in every module, every procedure, giving standard banners, a a functional description, list of the procedures in the module, what called what and by what, and on. It took longer to write that stuff that people ignore than the code.
URLs are an inteesting thought. Of course they are guaranteed to always be there!
Thursday, 17th July, 2008 at 8:19 pm |
I’m willing to bet that this was originally posted as an example of how not to comment code. Do I win?
(‘ Iterate loop. !!!!!!!! :-))
Thursday, 17th July, 2008 at 9:04 pm |
The Code came from here:
http://support.microsoft.com/kb/213438
Thursday, 17th July, 2008 at 9:13 pm |
“I’m still waiting to find a rational”
Mine is that I can reuse variable names of different types. I can use lUser for a long, sUser for a string, and so on. I don’t know if I’m rationalizing my religious orthodoxy or if it’s really useful, but I choose to believe the latter. And I always use i,j,k for loops.
“And worst of all”
I did a project a couple of years ago and used procedure-level headers (for the first time) that described the purpose, the arguments, and other seemingly relevant data. I found these very useful to get up to speed on the flow when I revisited the code, but didn’t find any of my inline comments useful. I can’t say if it was a time saver, though. I think it was about a wash. If I ever have to look at the code again, watch out. Productivity through the roof.
Thursday, 17th July, 2008 at 9:45 pm |
Thanks mrt
Sorry Rob it looks like you were not successful this time…
I’d rather see zero comments, everything scoped as tightly as possible, everything typed sensibly, reasonable names, and decent error handling. And all that would give better code for less effort than this ‘ministry of the bleedin obvious’ tat.
nooBs are going to be wondering where did ColsSelection etc come from? and what is it? part of Excel?
I tend to only comment stuff that doesn’t work as it should or that is a workaround. But when I have functions called ‘WorkAroundForIEBogShite(‘ I think the intent is pretty clear!
For teaching, copious comments are fair enough, especially if combined with good practice.
I wouldn’t fancy commenting every mod and proc, or maintaining something like that (maintain the code I mean, I would ignore the comments of course). Commenting callers is a workaround for having a crap editor that can’t list callers, and being unaware of MZ Tools.
Thursday, 17th July, 2008 at 9:49 pm |
Dick good point on the module stuff.
i do try and put a couple of sentences on what each mod or class is for.
Friday, 18th July, 2008 at 7:36 am |
I belong to the school of thought that comments only mean unfinished code.
With the module name, class name, procedure name and variable name, there is enough opportunities to explain what the code is trying to achieve without the need of a single comment. And this does not preclude the fact that procedure split in more granual ones adds more explanations whenever really necessary (in addition to being geared towards factoring and reuse).
Wednesday, 30th July, 2008 at 2:26 pm |
‘ Initialize RowsBetween equal to three.
RowsBetween = 3
Ha, the comment above is more confusing than the simple line of code:)