I find it odd that we as programmers spend so much time avoiding copy/paste and similar source duplication only to advocate such practices when it comes to commenting/documentation. The same inefficiencies and procedural problems as code duplication exist there...

Comments are useful to provide 'why' context or warnings to those reading code. Beyond that they're either fluff which obfuscates the code, misleading translations of code to english, or a coping mechanism to try and make up for poor naming and/or design by those too inept to correct them properly.

In my experience so far anyways... It's quite possible that such opinion will change as I see more and the tools available to us change and adapt.

Quote:Original post by Adam Hamilton
Imagine this: You need to add a certain feature in say OpenOffice. Now, wouldn't you prefer that the OpenOffice programmers commented their code so you would know where you need to be to add that certain feature.

I'd rather have them write readable code.
A common pitfall I often see (and also used to do) is to overgeneralize, and then having to comment how to use the generalism.

For example,
void SwitchMode(int mode);

would be much more readable if it was:
void SwitchToEdittingMode();
void SwitchToBrowsingMode();

1) SwitchMode() is not very descriptive of the method's functionality
2) obscure parameter, though that could be improved by using an enum


http://c2.com/xp/CodeSmell.html

Note that comments are listed as code deodorant.

KodeWarrior, you're an idealist right?
Ideally, everybody would write great self-explanatory code and comments would be redundant in all circumstances. Oh how I would love for this to always be possible, and do my best to achieve this on a daily basis!

Time to wake up and smell the real world though, where there do exist many situations that there is no better way to convey the information except through comments. Some random examples off the top of my head:

/*Whilst we would normally call Foobar to perform this function, we can't dothat here because we already have a lock on the Foo object.*//*This section of code is necessary to work around a problem whereby Foo's don'tdraw correctly under Foobar condifions. See www.thisIsAFakeLink.com/Foobar*//*These constants can be whatever you like as long as they are mutually prime.*//*We have to override Foo here as otherwise the base class will do a Foobar on us.*//*If you get foobar errors here, then make sure you are compiling with thecorrected version of foobar.h from $/Project/SubProject/SystemHeaders/*//*Warning: If you override foobar, make sure you still call the base classimplementation otherwise foo wil happen.*/

I was tempted to just fire off a "your opinion is wrong", but that wouldn't really be educational. So I'll waste 20 minutes instead.

My apologies if I cover ground that's already been covered in the thread.

Quote:States the obvious, therefore redundant.
//initialize variables
x = 10;

Agreed.

Quote:Proper layout standards and headers.
/************************************************************************************
Globals
*************************************************************************************/
Consider writing a standards document once, no need to copy and paste standards into every file. Should be obvious in language where external files are included, globals are declared, etc. Don't state the obvious.

I'm tempted to quip that you've never worked on a project with more than 20 lines of code. There comes a point where looking at a massive, homogenous blob of code just makes your eyes glaze over.

Speaking from repeated experience, when it's 4:30 AM, I haven't slept in three days, I'm stuffed full of artificial stimulants and terribly unhealthy food, and I have 36 bugs on my task list to solve in the next four days... I don't want to read code. Anything that lets me skip a chunk of code or skim it lightly is good. Anything that directs my attention to precisely where I need to look is very good.

For instance, we have a semi-formal convention on our team that the forward declarations in any header or code file are tagged with a comment: // Forward declarations. Yesterday I ran into a very convoluted compile error involving a massive legacy header file, because of a missing forward declaration. The comment tag jumped out at me (thanks to configurable coloration in my IDE), and it took roughly 3 seconds to find the section and fix the problem. Without that comment, I would have had to read a bunch of code. It would have wasted my time.

This instance, like many of your other nitpicky complaints, is a case that (in my opinion at least) clearly indicates that your post comes from somewhat arbitrary idealism rather than real-world experience. I don't mean to be cruel or insulting - it's just painfully clear that you haven't spent too much time in the trenches. That's OK - everyone has to start somewhere [smile]

Just bear in mind that, without the benefit of experience, your opinions are probably subject to revision. For that matter, even with experience, one should always be prepared to revise one's views if need be.

Quote:Alerts user to all assumptions made. Problem is with code, comments don't "fix" the problem. Who reads 'em anyway? Use ASSERT, CASSERT (compile time assert), or even make impossible with refactoring.
//does not work with negative numbers
f(int x){}
Comment should be eliminated and action taken to remove assumption.
f(unsigned x){}
or
f(int x){
ASSERT(x > 0 && "function assumes x > 0")
}
or return an error code
enum Errors{ERROR_OK, MUST_BE_POSITIVE};
Errors f(int x){}

What about assumptions that can't be tested via an assertion? For instance: // Construct a rotation from a float[12] array; we ALWAYS assume this comes from the Physics Lib and we need to transpose the rotation

Please show me the assertion that says "this float[12] array was originated by the ODE physics library."

Assertions are good tools, and they should be used. I'd even go so far as to say that they aren't used often enough in a lot of code out there. But they are not the ultimate solution to every problem.

Quote:Clarifies obscure code. Problem is with code, comments don't "fix" the problem. Who reads 'em anyway?
//hash for fast lookup
char a = b[0] ^ b[1] & c[1] | d[5];
Fix with refactoring and better naming
GetHash(...){...}
char hash = GetHash(...);
Example 2:
//qsort
void Sort(...){}
Better naming. Just a matter of moving the comments into the identifiers involved.
void QSort(...){}

This is the second time you've brought out this "who reads comments anyway" line. It sounds to me like you're generalizing from self - making assumptions about other people's work habits based on your own. This is not wise.

A programmer who does not, upon first encountering a piece of code, first study the code including all of its comments, is a piss poor programmer.

Comments exist for a reason. If you don't read them, maybe you need to reconfigure your IDE to make them stand out a bit more. Ignoring comments is foolish.


That said, the number one best use of comments is to clarify obscure code. For example:

	// Sample velocity at discrete time steps and use the Riemann sum to compute the offset.	// This is kind of ugly but it's the most effective way to handle sampled joystick input	// while still maintaining time independence, which is critical for coping with network	// latency and variant framerates in multithreading scenarios.

Without that comment, the following code would look like it could be replaced with a simple one-line multiplication, which is not the case. The distinction between a Riemann integration and a simple linear extrapolation is subtle and nearly invisible in the code because of the way the code is written. That comment is critical to understanding why the code does what it does.

Quote:Signals end delimiter. Shows lack of proper indentation.
#ifdef X
#ifdef Y
...
#else // Y
...
#endif // Y
#endif // X
Fix indentation and get an IDE that colors dead code so you can see if you don't have one.
#ifdef X
# ifdef Y
...
# else
...
# endif
#endif
or
*** Source Snippet Removed ***
Consider refactoring deep nesting instead.
*** Source Snippet Removed ***

I'm 50-50 on this one. Sometimes, in really big blocks of code (which, unfortunately, do happen, and are not always necessarily evil) it is nice to have some kind of reminder of what a particular } means. However, if the entire block of code fits on screen, indentation should be preferred to delimiter-comments, I agree.

Quote:Function return documentation.
//returns -1 if less than, 0 if equal, 1 if greater than
int compare(...){}
Use enumerations.
enum CompareReturns{CR_LESS, CR_EQUAL, CR_GREATER};
CompareReturns compare(...){}
or
//test for bad file
if(file == -1)
Use named constants
const handle BAD_FILE;
if(file == BAD_FILE)

In these particular cases, I agree with you. But that doesn't mean that it is always bad to document the return of a function, because sometimes it isn't obvious and isn't easy (or possible) to make it obvious: // Returns a velocity vector in local object space (Most of our vectors are in world-space in that particular module, so it's important to distinguish them.)

Quote:Reminders. Not the best way to remind.
//TODO: Add something here
Use pragma
#pragma message("TODO: Add something here")

Pragma is not portable. For a professional project that has to support multiple compilers, pragma is an unforgivable sin.

I spend probably 90% of my time looking at my IDE. With the exception of my browser (for accessing our bug database), most of my brain-time is spent looking at code. For minor reminders, dropping a TODO guarantees I won't forget it. There are plenty of small tweaks that are worth putting a TODO into the code but aren't worth filing a bug for.

In personal projects, I use inline TODO comments almost exclusively, because I can't be buggered to set up a bug database for every little 200-line utility I write (and I have a lot of such code).

Quote:
//if you modify this array, you will need to also modify array2
Use CASSERT.
CASSERT(sizeof(array) / sizeof(array[0]) == sizeof(array2) / sizeof(array2[0]) && "Must synchronize arrays")
//should not get here
Use ASSERT.
ASSERT(0 && "Should not get here")
//do not modify this variable
Use access modifiers, ASSERTS and remove dangerous global variables.

This is fine advice, when it is possible. There are times when the language just can't do what you want with an assert, at least not without some really excessive overhead: // WARNING: external data files rely on these values! Be sure to sync assets when modifying these constants.

Quote:Integrated documentation.
//this variable stores the current directory
char dir[MAX_PATH];
Rename variable so documentation will not have to be read.
char g_currentDirectory[MAX_PATH];
//function returns result of b + c in a
void f(int &a, int b, int c);
Rename identifiers so documentation will not have to be read.
void Add(int &result_out, int operand1, int operand2);
or safer
void Add(int *result_out, int operand1, int operand2);

Program that makes documentation automatically reads comments. External documentation should be redundant if you code correctly.

Bullshit. Good naming and good code style goes a long way, I won't debate that.

But comments have their place, and usually it's a lot more convenient to read the documentation inline with the code when you're working with the code. If I can see everything I need to know in my IDE, that's better than having to swap between my IDE and a documentation window. Context switches in the brain are productivity killers.

All of the information in this comment could be gleaned by carefully analyzing the code, but it would take literally days of study. Why should I force the next programmer who comes along (or myself in another year) to try and suss out the design decisions I made in the code, when I could just spell them out clearly?

Quote:Modified log. Shows lack of source control. Get it!

Fully agreed. However, if you don't comment your revisions in source control, you're evil. Every check-in should be documented thoroughly.

Quote:Remove unused code you might want to revert back to or worse, code that didn't work and got replaced. Shows lack of source control (if you don't have it, get it) or laziness (hit delete).
//memcpy(dest, src, count * 4);
memcpy(dest, src, count * sizeof(int*));

Again, this to me is evidence that you're not accustomed to working with really big projects. Consider a few cases, all of which happen to me regularly:

• I'm working on large revisions and can't make a checkin, but I need to temporarily rearrange things or make changes. I can't use source control, but I don't want to lose a block of code.


• I'm making a change which has a high chance of being reverted; it's polite to keep the original there so that the guy doing the reversion doesn't have to trawl through months of source history to find the change and undo it.


• I modified someone else's module and want to explain to them exactly why, and the easiest way to do it is in the context of the code. Typically, the expectation is that once the other programmer reads the comment, he'll remove it at his own discretion.


• We're in the middle of a big refactor, and the change will be checked in, but in three weeks when the refactoring is done someone needs to come back and re-enable functionality that is currently broken. This happens a lot when working in legacy code.

Quote:Comment obscure optimized code. This is assembly in which case does not qualify as "Bad C++ comment", or you underestimate your compilers optimizing capabilities.

I really just don't understand your reasoning here. Obscure optimizations (when they are necessary - and believe me, in the real world, they often are) are prime locations for comments. In the guts of a major 3D engine it is not uncommon to see things like "this is equivalent to the code Foo, but we did it this way because of Blah and it's X% faster".



Now for the "flip side":

Quote:Your job requires it. Don't get yourself fired.

If your job requires comments, it'd better have a damn good reason. If the reason isn't good, consider fighting to get the policy changed. Policies which waste programmer time are bad. It's not hard to convince management to change harmful policies when you express your concern in terms of "this is wasting my time, which means it's wasting your money".

On the flip side, if the requirement is good and sound, why would you be tempted to ignore it?

Quote:Document obscure external API calls.
//set valid processors this thread can run on. pass in bitmask of processors
SetThreadAffinity(...);
Distribute the API's documentation and look this stuff up. If it is as easy as hitting F1, don't comment. If bad documentation, comment is helpful. A valid question may be: "Why are you using an API with poor documentation?"

Reasonable principle, terrible example. SetThreadAffinity is pretty well documented.

On a personal project, or when you don't know who will be working with the code (OSS projects, etc.) it's worth commenting weird cases, such as:




However, this is different in a team scenario. If you have documentation that isn't easy to find (say, MSDN), that documentation should be collected in a shared point for all team members to access when they need it. Don't duplicate that documentation (the Don't Repeat Yourself principle). Point to it, sure, but never duplicate it.

In general, APIs/libraries/etc. or not, if you have really obscure code that isn't immediately obvious, comment it.

Quote:Comments to facilitate searching.
//***globals***

Err... doesn't this rather contradict your second point in the "comments are bad" section? I'm confused. (I think maybe this was an edit.)

Quote:My conclusion is that comments show deficiencies in code or lack of proper tools. When you have no control over the code, comments are ok.

No. Comments, when used properly, are a complement to code. Code does not exist for the purpose of communicating with computers. Code exists for the purpose of communicating with human beings. Comments complement this.

Quote:I'm interested in any exceptions to the following rules you can come up with. I would also like to take a poll. If you read this post, please vote so you don't skew the results. Just put at the start of your reply "I prefer comments" or "I prefer no comments". The I prefer no comments assumes the rules above are followed making the comments unnecessary.

I prefer programmers who I can trust enough that I don't have to be anal about whether or not they use comments in some particular scenario.

Quote:Original post by Oluseyi

Quote:Original post by Nathan Baum
That looks like it documents the public interface for client programmers. What do you think?

I think I don't read header files. I read documentation and let my IDE convince me that the header files match them.

But let me do a cursory check on my system at work:
Standard C++ Library - no.
Win32 - no.
ACE - no.
GSoap - no.

Let me examine some more libraries/headers that I tend to use (mostly at work):
Boost - no.
Python - no.
Perl - no
Xerces - no.

It would seem that your example is the exception, not the rule. Considering that it's an open source project, and we know how they tend to be about documentation, it's not surprising that they documented their interface in the header files rather than providing readily searchable hypertext (or, where they do provide hypertext, it's a machine extraction of those same comments).

But, please, feel free to assert that trawling through header files is more efficient, more maintainable and more client programmer friendly than providing separate documentation.

Personally i use machine extraction of comments to generate documentation for most smaller projects, simply because its convinient to write the API documentation while writing the code (or even prior to writing the code in some cases) without switching between applications. And then there really is no reason to remove those comments (even though they are redundant once proper documentation exist).