Worst Practice - Bad Comments
-
Comments posted to this topic are about the content posted at http://www.sqlservercentral.com/columnists/awarren/worstpracticebadcomments.asp>http://www.sqlservercentral.com/columnists/awarren/worstpracticebadcomments.asp
-
Andy - I agree with your view of bad comments. I do however, kick code back to our devs if they do not have PVCS header tags in the body of the stored procedure. I utilize an inhouse sproc that will compare all sprocs, functions and perms between the current instance and another instance to see what changes there are - really helps when the system acts differently between platforms (dev, build, qa, stage and prod)...
My 2 cents worth on comment blocks with version, author, etc in them.
-
I agree in part. I like top/bottom borders for "major sections" (define for yourself what constitutes major). I don't have many sprocs with major sections, but I do have a number of maintenance or build scripts with major sections, and I like having the borders to highlight that fact. Perhaps this is anal of me, but I have a template with the borders measured just long enough to go all the way across the page if it is printed, and not wrap to a second line. But I agree with getting rid of left/right borders. Too much of a pain.
I like a comment block at the top that includes a brief narrative and a revision history, but if the history is really getting lenghy in comparison to the rest of the code, perhaps it should be truncated or summarized. I have seen, and do NOT like, date-stamped comments about each minor change within the code. Similar to the "old code" comment, it tends to overrun the code and make it hard to follow what is really going on. When I want to know exactly what changed, I go back to my version control system and run a Diff comparison.
-
I insist on header blocks because a well written header tells the uninitiated at a glance what the proc/function is supposed to do.
I do keep a history in the proc, but truncate it if it gets too excessive.
I like to detail
- Variables used
- Arguments Used
- Tables/Views used
- Procs/Function used
There is nothing worst than scrabbling around a large stored proc trying to figure out what variable @x is supposed to represent.
If you are looking at an app regularly or you wrote the app then header comments are of no particular advantage. If, however, you are a new comer to the app, or you haven't worked on an app for some time the use of headers becomes a godsend.
-
I agree with the bulk of the article, however, headers are in my opinion essential.
To have a description of the purpose of the procedure, alongside the code, is the one piece of documentation that is not going to be lost.
-
Also like the comment header. Agree with David.Poole on this. And we agreed to cut out all changes in between major versions / releases.
This keeps the changelog nice and short.
-
Good comments!
Sorry, couldnt help myself!
One thing I've been considering trying is adding a 'Used By' comment at the top that would indicate any app that calls the proc. If kept up, you'd know instantly the real dependencies, not just the ones SQL knows about (and it doesnt always have them all even for procs that call other procs). Anyone tried anything like this?
Andy
-
I agree with all the reader comments on headers and their importance - but essentially I wanted to mention another bad commenting practice that is related to "neatness" and readability - I have come across both code & comments where the developer has used an unhappy & random mix of capital letters and lower case letters (eg: FIRST_name, last_NAME, middle_initial etc...) and is consistently inconsistent throughout the code. The code itself is excellent, but the readability is so painful (& for me traumatic) that I can't seem to go beyond just the sight of the whole thing.
So for all those out there who randomly use lower case and upper case and argue that the code works just fine regardless of what you use - refer to Andy Warren's line where he says that neatness contributes to readabilty - & readability finally contributes to maintenance - Just like you wouldn't trash your house with litter, look upon your code the same way - take pride both in it's functionality AND appearance!!!
**ASCII stupid question, get a stupid ANSI !!!** -
Good point on readability.
One problem with comments is that they can be displayed in a variety of fonts so if you have anything lined up results can be unpredictable.
Ditto indentation of code.
I've tried a recommended display font line in the header.
I tried a "Used By" comment box. The problem is that there has to be a gate keeper if this is to be maintained.
On a large project I tend to develop a series of interface stored procs and publish their intended use to my developers, they can then use these as they require, but as they can use it for any number of routines this makes a "Used By" comment difficult to maintain.
Really, keeping track of who uses what has to be an end to end process practised by the organisation, not just one or two individuals in a team.
Basically in small organisations you can have an "ad-hocracy". Operations under ad-hocracies are fast and flexible. The problem comes when you grow beyond a certain point ah-hocracies implode.
-
Again, have to agree with David. 'Used By' comments are usefull, but it's a pain to keep them up to date. And if you can't trust them, what's their use.
In that respect, I tend to look at procedures (and other SQL scripts) as 'ordinary OO' code, with the appropriate lessons to follow on high cohesion / low coupling.
If we are developping for a large project, we try to add an indication of 'module' to the name of each object. One person is responsible for each module, so he makes sure nothing breaks inside a module after changes.
For 'inter module' communication, we design an interface, consisting of views and insert / update / delete procedures, that has to be adhered to.
If someone has to add a field to a table, he has to make sure all of the interface objects will keep their functionality. Hurray to default values and ban all 'SELECT * FROM ORDER BY 1' kind of stuff
This kind of set-up has worked for our biggest projects, involving up to four different firms with in total some twenty concurrent developpers in SQL, C++, VB and Crystal / Cognos.
-
Is there anyone out there at the other end of the scale. With a live constantly evolving production system with next to no comments and no documentation? Yes I know its 2003 .....
Nigel Moore
====================== -
Any suggestions on the best approach to tackle such a situation if you inherit it?
Nigel Moore
====================== -
If it works its obsolete
It depends on the nature of the evolution. If it is constantly trying to implement new stuff then this suggests that you are not able to implement proper change control/testing procedures.
It also suggests that you are dependent on individuals with key knowledge of the system.
I say this because if you work in teams then you simply can't do without good documentation. The team cannot function without knowledge share. Knowledge share includes documentation
Trying to implement good practice retrospectively is hell.
Ideally, you need to get agreement to slow down the evolution and have set release timetables, but you need management to buy into this.
Sell it as
- Long term reduced cost
- Improved quality
- Long term Increase productivity
To use a military analogy, after every advance you need to consolidate your position and make sure that the supply lines do not become over extended and therefore vulnerable.
Think of the supply lines as being the path to bring a new team member up to speed on the system
-
Some of the early products are this way. They changed almost everyday depending on who called you and none of the code had documentation or comments so you were left reading it. Some of the older code is still out there without comments but about a year ago we got everyone into the habit of doing comments if no documentation (self documenting
). All and all things have gotten much better. It takes me about and 8th the time to figure out what to change to correct most situations and we forced the end users to go thru a project manager who would validate need, time and priority (which priority was the key, no more now stuff). But as David states it is a pain.I agree with everyone here on comment header blocks with details of who, when and why in them. Maybe MS will move this into SQL like they have done with column and other comments so you don't have to do this but I doubt it.
-
Headers are essential. They not only indicate who made a change (provided the author still works with you) but also tie a change to a work order here, allowing you to find the original requirements to see the reasoning behind it.
Viewing 15 posts - 1 through 15 (of 37 total)
You must be logged in to reply to this topic. Login to reply