Sign in to follow this  
KodeWarrior

C++ comments are bad!

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
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
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
Nathan:
1: Rule: Don't call any functions if you don't know what they do and how to call them. So the question becomes, do you merge your code and documentation or do you keep them separate? MSDN is an example of separate documentation.

Edit:
On second thought...
enum Errors{ERROR_OK, MUST_BE_NEGATIVE, MUST_BE_REL_PRI};
Errors g(x)
Errors h(x, y)
or
g(x, Errors &errorCode_out)
h(x, y, Errors &errorCode_out)
Don't assume people actually read comments.

2 and 3: The question is really do you think the commented version is better in Omid's case?

[Edited by - KodeWarrior on March 20, 2007 8:14:42 PM]

Share this post


Link to post
Share on other sites
Quote:
Original post by Nathan Baum
Quote:
Original post by KodeWarrior
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).

wouldn't the better thing to do is:

//function pointer - let's pretend ^^ means function pointer
void sort^^(T[] array)

//assign specific sort function to sort
sort^^ = quicksort();

Quote:
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().

Oh geez that's funny. [grin]

Share this post


Link to post
Share on other sites
Quote:
Original post by Oluseyi
Quote:
Original post by KodeWarrior
MSDN is an example of separate documentation.

Uh, no. MSDN documents public interfaces for client programmers, not implementation details. MSDN is not analogous or comparable to comments in source code.

Not even comments which document public interfaces for client programmers?

Share this post


Link to post
Share on other sites
Quote:
Original post by KodeWarrior
Nathan:
1: Don't call any functions if you don't know what they do and how to call them. So the question becomes, do you merge your code and documentation or do you keep them separate?

Which is more likely to result in up-to-date documentation?
Quote:

2 and 3: The question is really do you think the commented version is better in Omid's case?

Sort() is better than MergeSort() for the reasons given. I don't care whether there's a comment on Sort noting that it uses MergeSort internally.

Nobody who writes a GCD function and calls it specialFunction deserves to live. The comment is irrelevant in this case. OTOH, A GCD function named GCD which notes in a comment that it uses Euclid's algorithm, on the other hand, is fine and dandy.

Share this post


Link to post
Share on other sites
1) Use of unsigned types does enforce constraints because you should be compiling on max warnings with "treat warnings as errors" enable which will prevent such "mistakes"

2) An example of useless comments that keep people sane (and hence arnt really useless):

struct no_elem { };

#define TUPLE_MAX_ELEMS

/*
template<typename T0 = no_elem, typename T1 = no_elem, ..., typename Tn = no_elem>
struct tuple
{
template<unsigned int item_num>
T[item_num]& get()
{
static_assert(item_num < max_items);
return element[item_num];
}

private:
typedef T0 value_type_0;
typedef T1 value_type_1;
...
typedef Tn value_type_n;

value_type_0 element_0;
value_type_1 element_1;
....
value_type_n element_n;
};
*/

template<BOOST_PP_ENUM_BINARY_PARAMS(TUPLE_MAX_ELEMS, typename T, = no_elem BOOST_PP_INTERCEPT)>
struct tuple
{
template<unsigned int item_num>
getter<tuple, item_num>::type& get()
{
BOOST_STATIC_ASSERT(item_num < TUPLE_MAX_ELEMS)
return getter<tuple, item_num>::get(*this);
}

private:
template<typename Tuple, unsigned int item_num>
struct getter;

BOOST_PP_REPEAT(TUPLE_MAX_ELEMS, TUPLE_GEN_ELEMENT_LIST, ~)

BOOST_PP_REPEAT(TUPLE_MAX_ELEMS, TUPLE_GETTER_SPECILIZATION, ~)
};

#define TUPLE_GEN_ELEMENT_LIST(z, n, data) \
typedef BOOST_PP_CAT(T, n) BOOST_PP_CAT(value_type_, n); \
BOOST_PP_CAT(T, n) BOOST_PP_CAT(element_, n);

/*
template<typename Tuple>
struct getter<Tuple, n>
{
typedef typename Tuple::value_type_n type;
type& get(Tuple& tuple)
{
static_assert(element exists);
return tuple.element_n;
}
};
*/

#define TUPLE_GETTER_SPECILIZATION(z, n, data) \
template<typename Tuple> \
struct getter<Tuple, n> \
{ \
typedef typename BOOST_PP_CAT(Tuple::value_type_, n) type; \
BOOST_STATIC_ASSERT(!boost::is_same<type, no_elem>::value) \
type& get(Tuple& tuple) \
{ \
return BOOST_PP_CAT(tuple.element_, n); \
} \
};




3) Example of useful comment

template<typename T>
struct vector
{
void push_back(const T& value)
{
// if the vector is full double the size inorder to satisfy
// the amortized constant time push_back requirment
if (...)
{
...
}
...
}
};

Share this post


Link to post
Share on other sites
Quote:
Original post by Alpha_ProgDes
Quote:
Original post by Nathan Baum
Quote:
Original post by KodeWarrior
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).

wouldn't the better thing to do is:

//function pointer - let's pretend ^^ means function pointer
void sort^^(T[] array)

//assign specific sort function to sort
sort^^ = quicksort();


I would say no. You'd have to make sure sort was assigned appropriately every time you called it.

Also, I usually want to avoid having to select a specific algorithm: I'd rather have the program select an algorithm based upon the characteristics I require.

e.g.


enum SortMode
{
SORT_STABLE = 1, // Preserve ordering of "equal" elements.
SORT_PRESORTED = 2, // Data is mostly sorted already.
};

void sort (T[] array, sign comparator (const T &, const T &) = operator<, SortMode mode);

Share this post


Link to post
Share on other sites
Quote:
Original post by Julian90
1) Use of unsigned types does enforce constraints because you should be compiling on max warnings with "treat warnings as errors" enable which will prevent such "mistakes"

Assuming your compiler considers implicit casting from signed to unsigned a warnable offense. GCC doesn't. Mixing signed and unsigned arithmetic: yes. Implicit casts: no.

Share this post


Link to post
Share on other sites
KodeWarrior: It seems to me that it is quite easy to bag out comments with the examples you gave. Any one can do it.

// Increment i
++i;

See... But there will come a time in your life (if you're serious about programming) when you will come across a chunk of code that you/or others have written and wonder what exactly the code is trying to do. If you cam across an algorithm, you would want the code to be commented so that you knew what the algorithm was trying to do.

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.

Good comments are good and bad comments are bad...

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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 do
that 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't
draw 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 the
corrected version of foobar.h from $/Project/SubProject/SystemHeaders/
*/
/*
Warning: If you override foobar, make sure you still call the base class
implementation otherwise foo wil happen.
*/

Share this post


Link to post
Share on other sites
Guest Anonymous Poster
Quote:
Original post by KodeWarrior
Bad uses of comments:

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



How about......no?

Here's my exception: you work as a software engineer, and constantly work on different projects, and often times you work on applications with code that exists prior to your working on the project, and you won't be the last to work on it either.

If not commented, you have no clue what anything does and you have to waste time to fix it. If commented, you will have a general understanding. When you write your own code, comment it for the next guy to read and understand. If comments show flaws to others, good. Then the others will see it, and fix them.

Share this post


Link to post
Share on other sites
I've made my position clear in the previous thread on this subject linked to already.

Comments used as a replacement for self documenting code are bad.
Comments which duplicate what's already shown in the code are bad.
Code is limited, however, and sometimes comments are a necessity.

If we work upon the assumption that a misusable tool is bad, then C++ by definition is bad, and the whole question of wheither or not C++ comments in specific are bad becomes fairly moot.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this