Jump to content
  • Advertisement
Sign in to follow this  
KodeWarrior

C++ comments are bad!

This topic is 4170 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

Bad uses of comments: States the obvious, therefore redundant. //initialize variables x = 10; 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. 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){} 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(...){} 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
while(){
   for(){
      if(){
         do{
            if(){
            }//if
         }while();
      }//if
   }//for
}//while


Consider refactoring deep nesting instead.
void Inner(...){
   if(){
      do{
         if(){
         }
      }
   }
}
while(){
   for(){
      Inner(...);
   }
}


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) Reminders. Not the best way to remind. //TODO: Add something here Use pragma #pragma message("TODO: Add something here") //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. 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. Modified log. Shows lack of source control. Get it! 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*)); Comment obscure optimized code. This is assembly in which case does not qualify as "Bad C++ comment", or you underestimate your compilers optimizing capabilities. The flip side. Valid uses of comments. Your job requires it. Don't get yourself fired. 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?" Comment out single line of test/debug code. Prefer #if 0 though over /**/ since inline comments can't nest. Also consider using #ifdef _DEBUG instead of comment. Will be less error prone if you don't need in release testing/debugging. Comments to facilitate searching. //***globals*** Temporary comments as placeholders for code while constructing. 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. 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. Edit: added another example [Edited by - KodeWarrior on March 21, 2007 6:36:45 PM]

Share this post


Link to post
Share on other sites
Advertisement
not c++, a high level language! so if i ever make a comment the code is bad, right?


def appendleft(self, iterable):
"""
To avoid redundant and counterintuitive packet header bit
insertion user code like the following:

packet=Packet()
#add header of "100"
packet.appendleft(0) #push the rightmost bit first
packet.appendleft(0) #then work to the left
packet.appendleft(1) #finally the rightmost bit

appendleft has been modified to additionally accept an
iterable datatype, such as a list or string, like so:

packet.appendleft("100")
"""
try:
for item in iterable[::-1]:
super(Packet,self).insert(0,self.bitify(item))
except TypeError:
#unsubscriptable object, or iteration over non-sequence
super(Packet,self).insert(0,iterable)


...right?

edit: disclaimer: i got bored, so i didn't read your whole post.

Share this post


Link to post
Share on other sites
I prefer comments.

Quote:
Original post by KodeWarrior
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.

Should be. Frequently isn't. If you need to declare a global, it's easier to search for "**GLOBALS**" than it is to go to the top of the file and hunt down for where the globals start.
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")
}

The implicit cast from signed to unsigned breaks the first solution.

In the second solution, it is more likely that automated documentation tools will be able to do something useful with the preceding comment than with the assertion. Also, a competent IDE 's StupidiSense will be able to show you the comment, so you may not even need to check the documentation to know what gotcha's are involved.

A programmer shouldn't have to read the source to a function to know what constraints apply to its arguments.
Quote:

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

What does #pragma do? Does it cause the message to show up on a task list in automatically generated documentation? Does it cause the compiler to start up a Towers of Hanoi simulation?
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")

The assert only covers one possible modification to the arrays, so it isn't necessarily a substitute for the comment.

Share this post


Link to post
Share on other sites
I think comments are a must. Writing large/complex pieces of code without comments is like drawing someone a map with no location names and no legend.

Share this post


Link to post
Share on other sites
I prefer comments.

So you enjoy wasting time on looking through code to figure out the mathematical algorithm rather than have it written out in a short comment section above?

Share this post


Link to post
Share on other sites
Writing clean, self-evident code is a good thing, and should be encouraged, but it really only explains what is happening. Comments on the other hand can explain WHY you made the algorithm this way, or HOW to use this piece of code. Also, if you don't comment heavily optimized code or the use of an obscure algorithm, large men will beat you savagely.

Share this post


Link to post
Share on other sites
Nathan:
Changed valid uses of comments for first example in original post.

In the second example, StupidiSense will pop up f(unsigned x) letting user know variable expected should not be negative.

Omid:
What do you prefer?
//uses the merge sort alg
void Sort(...){...}
or
void MergeSort(...){...}
Calling:
//Euclid's GCD
x = specialFunction();
or
gcd = EuclidsGCD();

Horatius83:
Good points, updated original post.

NerdInHisShoe:
You might consider refactoring those "blocks" into well named functions. Call the functions whatever the comments might have been.

[Edited by - KodeWarrior on March 20, 2007 8:29:43 PM]

Share this post


Link to post
Share on other sites
Quote:
Original post by KodeWarrior
In the second example, StupidiSense will pop up f(unsigned x) letting user know type expected should not be negative.

That would work, but it isn't a general solution to argument constraints. What if in g(x), x must be negative, or a multiple of four? Or in h(x, y), x and y must not be relatively prime?
Quote:

What do you prefer?
//uses the merge sort alg
void Sort(...){...}
or
void MergeSort(...){...}

Sort(), certainly. I don't usually need to care which algorithm is being used. Using MergeSort() means that if I later discover QuickSort behaves better with my data, I have to change the name used in every call. If I should happen to care what algorithm is used, it will usually be because of its externally visible features. I may wish to choose between Sort, StableSort or LinearSort (yes, linear sorts exist, but only for certain types of data).
Quote:

Calling:
//Euclid's GCD
x = specialFunction();
or
gcd = EuclidsGCD();

Neither; I prefer GCD(). Why do I need to know what algorithm the function is using to calculate the GCD? I wouldn't call a function SquareRootOfSumOfXSquaredYSquaredZSquaredVectorLength().

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

We are the game development community.

Whether you are an indie, hobbyist, AAA developer, or just trying to learn, GameDev.net is the place for you to learn, share, and connect with the games industry. Learn more About Us or sign up!

Sign me up!