"Don't use comments"

[Phasma Felis]

Had a conversation with a coworker today who said that you should never leave a comment explaining what code does--the function should be clear from reading the code; if it's not, you need to rewrite, not comment.

I've been thinking about this, and I really don't like it. The principle "code should be as clear as possible" is an excellent one, but "code should be as clear as a comment" is only possible if the reader speaks Ruby (or whatever) as fluently as English. It seems to me that that's not true for a lot of coders, and the ones for whom it's least true (i.e. newbies) are the ones who need it most.

Thoughts?

[Dopefish]

That seems like an excuse not to comment, rather then a reason not to.

There is almost certainly multiple ways to code any given thing in the same language, and if the way you happen to do it is different then the readers route, they could very well be very confused, even if the code is very straight forward to you. (This applies even if the reader is perfectly fluent in the language, which is of course a tall order.)

Furthermore, if the code ends up being particularly large or does anything particularly complex, even if you can follow every little bit individually, people don't necessarily want to follow through every little bit of your code just for things to make sense, when suitable commenting could remove the need for that entirely.

Often times I do feel like I'm wasting my time with my commenting since "it's clear from the code", but if that code needs to be dealt with by another person, it can definitely be worthwhile. While for small personal things that you're sure will never be seen/used by anyone else it may be fine to slack on it, but it's a good habit to build even then.

Not to mention something that is clear and obvious to you now, may not be clear and obvious in a years time, after which you may have since switched to a completely different language and are used to thinking in a completely different mindset.

[ConMan]

If the code is well-written, then it should always be clear *what* the code is doing at a particular point. What is not always clear is *why* it's doing that, which is where comments are useful. So you're incrementing x? Great! What's x? Why does it need incrementing? What was its previous value? What value(s) do we expect it to have at this point?

This is even moreso the case when you're looking at one small component of a larger system, with little to no understanding of the whole. Where were these variables declared? With what format and initial values, if any? When is this function called, what does its parameters represent, what is it expected to output, and what other things might it do before it exits? What kind of exceptions might it throw, and where do they get handled?

[Inglonias]

Yeah, looking at my old, uncommented code embarrasses me now. Commenting makes it so much easier to come back to a project after I "finish" it three months ago.

It is imperative if you are coding jointly with others. The why is just as important as the what in these cases. They need to know why your code is written the way it is.

I personally feel that there is no excuse under the sun not to comment your code.

[Xanthir]

Yup, others got it. A good comment captures the "why", not the "what". Clear code explains the "what" naturally, but "why" might be based on arcane details of an API you're interfacing with, or work around bugs that you discovered, or have some tricky details due to performance concerns, etc.

[PM 2Ring]

As I said in the recent Do you like documentation in code, and if so, how/when? thread:

It may be necessary to use comments to explain what some code is doing, and perhaps to explain why it's doing it. They should not be required to explain how it's doing it, unless the code is using some obscure but efficient algorithm. In general, use of arcane clever code is discouraged, since it breaks Kernighan's rule: "Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it."

[rpenido]

Your code must be clear in first place and you can't solve that with a lot of comments.
I used to comment code when I need to do something that doesn't make sense, and will consume a lot of time to figure out again what's happening. Like "//I'm not sure why this works but it fixes the problem.". Really common when you're using third party libraries and need some "workarounds".

More info:
http://c2.com/cgi/wiki?ToNeedComments

Bad code

Code: Select all

int x = getMachineStatus();

if (x == 4)
{
  doSomething();
}


Bad code, commented

Code: Select all

int x = getMachineStatus();

if (x == 4) // Check if the machine is running
{
  doSomething();
}


A way better

Code: Select all

enum MachineStatus {Idle, Running, Alarm};
MachineStatus currentStatus = getMachineStatus();

if (currentStatus == MachineStatus.Running)
{
  doSomething();
}


Can you post a code example of what are you talking about ?

[eskarel]

Your coworker is right, though not in the way you're thinking, and possibly not in the way he is thinking.

Comments probably indicate badly designed code(though not necessarily your badly designed code), documentation on the other hand indicates a good coder. Don't write crap that can't be understood, do explain what methods, classes, properties, etc are supposed to do, even if they have sufficiently descriptive names.

[Yakk]

Code: Select all

    enum MachineStatus {Idle, Running, Alarm};
MachineStatus getMachineStatusImproved()
{
#pragma push ignore deprecated getMachineStatus
    MachineStatus currentStatus = getMachineStatus();
#pragma pop ignore deprecated getMachineStatus

    // There is a problem with the legacy API where Idle is sometimes returned
    // on an Alarm if we are in a particular corner case (describe corner case here).
    // There are other consumers of the API that rely on that corner case working,
    // and we are unable to deal with it at this time, because of problems X, Y
    // and Z.  The decision was taken to fix it in an extended function.  The
    // original function is now deprecated.
    if (currentStatus == MachineStatus.Idle)
    {
      dealWithCornerCase();
    }
    return currentStatus;
}


I guess we could use a really long variable name for currentStatus instead of a comment like that.

[Phasma Felis]

Can you post a code example of what are you talking about ?

Let's see, I was recursing over a directory tree and doing something different with each entry depending on whether it was a file or a subdirectory. The inner block was something like this (Ruby):

Code: Select all

if entry.file?
  <<< do something >>>
elsif entry.directory?
  <<< do something else >>>
end

We talked briefly about how "entry.directory?" was unnecessary, since if it wasn't a file it had to be a directory, so I started to change it to this:

Code: Select all

if entry.file?
  <<< do something >>>
else      # entry is a directory
  <<< do something else >>>
end

He quickly said that if I needed a comment I should leave it in the original form, because code that needs comments to explain it is bad code. In this case I don't mind leaving it that way, but I do like to explain strange bits of code and generally describe functional blocks in comments, so.

(Although perhaps I should break functional blocks down into actual functions more than I do, and just document the functions.)

[Ben-oni]

I'm firmly in the camp of "Good code needs no comments" (recognizing, of course, that the converse is most certainly false).

That being said, there are times when even the best of programmers ought to be using comments. Personally, I find them necessary when the language actively works against me, making it difficult to implement abstract concepts. C doesn't let me write inline functions. Java makes it painful to write inline classes. Without high-level constructs, the so-called "complex" systems actually are complicated to assemble, so comments are needed.

Then again, working programmers don't have the time to refactor their code until the comments are redundant, so the insistence that there should be enough comments to be able to maintain the code has some truth to it.

As for "What vs. Why", my gut tells me that the Why is always "Because this is the problem being solved". If done well (which I recognize is not always practical), the What (the solution to the problem) looks like the problem statement, so the Why is redundant.

In particular, algorithms based upon obscure mathematical theorems (by some definition of obscure) should be isolated from the rest of the code and well documented — not commented. Put the proof in the documentation, or direct the reader to a relevant paper.

Here's a piece of code I wrote recently. Tell me, where should I comment?

Code: Select all

-- Project Euler, Problem 55
import Data.List (unfoldr)
import Text.Printf (printf)

numToDigitsRev = unfoldr (\n -> if n < 1 then Nothing else Just (n `mod` 10, n `div` 10))
digitsToNum = foldl (\n d -> n*10 + d) 0
reverseInt = digitsToNum . numToDigitsRev
isPalindrome n = let ds = numToDigitsRev n in ds == reverse ds
isLychrel x = not $ any isPalindrome $ take 50 $ drop 1 $ iterate (\x -> x + reverseInt x) x

main = printf "There are %i Lychrel numbers below 10000.\n" $ length $ filter isLychrel [1..9999]

[Ben-oni]

Can you post a code example of what are you talking about ?

Let's see, I was recursing over a directory tree and doing something different with each entry depending on whether it was a file or a subdirectory. The inner block was something like this (Ruby):

Code: Select all

if entry.file?
  <<< do something >>>
elsif entry.directory?
  <<< do something else >>>
end

We talked briefly about how "entry.directory?" was unnecessary, since if it wasn't a file it had to be a directory, so I started to change it to this:

Code: Select all

if entry.file?
  <<< do something >>>
else      # entry is a directory
  <<< do something else >>>
end

He quickly said that if I needed a comment I should leave it in the original form, because code that needs comments to explain it is bad code. In this case I don't mind leaving it that way, but I do like to explain strange bits of code and generally describe functional blocks in comments, so.

(Although perhaps I should break functional blocks down into actual functions more than I do, and just document the functions.)

I'd go with the former. Mostly because entry.file? and entry.directory? might not be exhaustive tests. What if you want to do something different on symlinks?

[EvanED]

I'd go with the former. Mostly because entry.file? and entry.directory? might not be exhaustive tests. What if you want to do something different on symlinks?

Agreed. In fact, I'd prefer to see both tests and then an else case which asserts or throws an exception or something. The statement that "if it's not a directory then it's a file" is just false; depending on your file system there may also be symlinks, sockets, fifos, character and block devices, whiteouts, reparse points, and possibly others.

The other thing that I've done when I've had something like that is something like

Code: Select all

if choice1:
    ....
else:
    assert choice2
    ....

I think my general rule is "if you can turn your comment into code reasonably well, then do so; if the comment repeats something which is self-evident about the code, remove it"; my suggestion changes your "entry is a directory" comment into code, as does the version you don't like.

[Thesh]

Use comments when useful. For example, I ran across this today:

Code: Select all

if (iterations % 100 == 0)
{
    //Sleep for 1 minute after sending 100 emails
    Thread.Sleep(60000);
}


That comment was completely useless. This would have actually been helpful:

Code: Select all

if (iterations % 100 == 0)
{
    //Because my helper class new connection to the SMTP server for every email sent
    //and .NET didn't provide a Close/Disconnect/Dispose method for SmtpClient
    //we need to sleep every minute so the GC can close the connections I open
    Thread.Sleep(60000);
}


If I didn't have to ask around to find out the history, I would have just added the two lines of code to initialize an SmtpClient class and add the credentials, commented out his method and replaced it with SmtpClient.Send and the problem would have been solved much faster.

[Ben-oni]

If I didn't have to ask around to find out the history, I would have just added the two lines of code to initialize an SmtpClient class and add the credentials, commented out his method and replaced it with SmtpClient.Send and the problem would have been solved much faster.

Well, there's no solution for bad programming. Would it not have been better for

Code: Select all

if (iterations % 100 == 0) GC.Collect();

to have been written instead? At least explicitly calling the GC, even if it doesn't explain the problem, indicates you need resources to be released.

[Thesh]

Instantiating one instance of the SmtpClient class is the better solution, and the recommended comments that I put in requires knowledge of why there was a problem. In reality, he probably didn't know what was wrong.

[Sc4Freak]

That doesn't sound right... if a class holds onto some resources (unmanaged or otherwise) that need deterministic destruction, it should always implement IDisposable. It sounds like an oversight.

[Thesh]

That doesn't sound right... if a class holds onto some resources (unmanaged or otherwise) that need deterministic destruction, it should always implement IDisposable. It sounds like an oversight.

Yeah it was an oversight. Microsoft fixed it in .NET 4.0.

[Carnildo]

Had a conversation with a coworker today who said that you should never leave a comment explaining what code does--the function should be clear from reading the code; if it's not, you need to rewrite, not comment.

Have him re-write the following code to explain why it's formatted as

Code: Select all

if(window != NULL && window->callback != NULL)
{
    long result = window->callback(w,x,y,z);
    return result;
}

rather than the more common

Code: Select all

if(window != NULL && window->callback != NULL)
    return window->callback(w,x,y,z);

Spoiler:

Hint: the second form, under the right (rare) conditions, triggers a bug in the optimizer of GCC 3.4.x that turns the function call into a jump to address 0x0.

Then have him re-write the following code to make it self-evident that the call to CFRelease is mandatory, even though it violates the "get" rule of the CoreFoundation API:

Code: Select all

if(noErr == ATSFontGetName(font, kATSOptionFlagsDefault, &fontName))
{
    /* Do something */
    CFRelease(fontName);
}

Spoiler:

If you're not familiar with it, the "get" rule is that any reference-counted object returned from a function with "get" in the name must not be CFReleased unless you've explicitly CFRetained it; violating this rule can cause a use-after-free crash. The converse is the "create" rule: any reference-counted object returned from a function with "create" or "copy" in the name must be CFReleased when you're done with it; violation of this rule will cause a memory leak. Apple should have named the function "ATSFontCopyName()", but they didn't.

[lorb]

What about this? It's not exactly coding as my example is css but i could see the same happening in code.

Code: Select all

background-color: #ffb300; /* orange */


[rpenido]

What about this? It's not exactly coding as my example is css but i could see the same happening in code.

Code: Select all

background-color: #ffb300; /* orange */


I don't think it's bad.
The problem is that, with high probability, someone will change the color and do not update the comment.

Code: Select all

background-color: #00b3ff; /* orange */


After the change, you will spend some hours trying to figure out WHY the background is BLUE when the CSS said its ORANGE (at least on the comment).

[Breakfast]

What about this? It's not exactly coding as my example is css but i could see the same happening in code.

Code: Select all

background-color: #ffb300; /* orange */


If your example appears in code you should do something like:

Code: Select all

const Color _orange = #ffb300

This way, you can have many places that reference the _orange variable and if you ever wanted to change the color of _orange you would only need to make the change in one place and it would propagate to all of the references. This leads to the code being self-documenting.

[hotaru]

What about this? It's not exactly coding as my example is css but i could see the same happening in code.

Code: Select all

background-color: #ffb300; /* orange */


it'd be better to just use "orange" (http://www.w3.org/TR/CSS21/syndata.html#value-def-color) unless you really need "#ffb300". if you really need "#ffb300", your comment should explain why.

[Xanthir]

What about this? It's not exactly coding as my example is css but i could see the same happening in code.

Code: Select all

background-color: #ffb300; /* orange */


If your example appears in code you should do something like:

Code: Select all

const Color _orange = #ffb300

This way, you can have many places that reference the _orange variable and if you ever wanted to change the color of _orange you would only need to make the change in one place and it would propagate to all of the references. This leads to the code being self-documenting.

Or, once everyone implements CSS Variables, just do that directly in CSS.

[troyp]

Completely self-documenting code is an ideal. In the (impossibly) perfect language, I think you could always write code in a way that naturally makes it clear not only what you're doing, but why. In reality, you're usually going to need to comment code for the latter reason and sometimes for the former as well, particularly in less expressive languages.

As far as the Phasma Felis' example goes, I agree with EvanED 100%. I don't really care if you add an elsif*, but if you do, there should be an else raise. If you just use the single else statement, you can simply replace the comment with an assert and you have self-documenting - and more reliable - code. More generally, though, I don't really like statements like "you shouldn't do that because code with comments is bad code". They seem like an excuse to not have to exercise your own judgement by slavishly following rules.

* "elsif"! Bloody Ruby! Either drop a letter or add one, FFS.

[Maelstrom.]

I wrote some code today that exported a list of Orders from one third-party application to a CSV file in a strict, third-party format. There were three different formats each line could conform to (Consignment, Article, and Goods lines), which had to be ordered in a specific manner. A Consignment was the main entity, but each Consignment could have multiple Articles (all the Article lines after a Consignment line belonged to that Consignment). Each Article could have one Goods line. Goods lines were required for international shipments, but not for domestic shipments.

International shipments required different data than domestic shipments, such as the Goods line. International shipments could only have one Article, while domestic shipments could have multiple Articles. They required a different package type to domestic shipments, and also required a phone number.

This CSV format stated that header rows were NOT allowed, so self-documenting code by including headers was out. About a third of the entries were optional, and not required by the application I was writing, so these were blank.

I honestly do not know how I could have written this exporter to be clear with out a liberal sprinkling of comments. I had one bug with the output where I had missed a (blank) field, and so all columns after it were offset by one. I did not catch it earlier, as I had not identified each column with an identifier (see: no headers). I added a comment above each column or column group in the code indicating the expected column it would appear in, to keep debugging easy. I commented on lines that were a single 'N'/'Y', to say why this was appropriate. I added the column header as a comment after blank columns, so I could keep track of things, and easily make that column non-blank in the future if it became relevant.

The Ordered product weight was in grams, but the CSV output needed to be in kilograms. Both fields were called 'weight'. While you could guess that

Code: Select all

weight / 1000.0

is a unit conversion, guessing in code is bad. Clever variable naming could have helped, but I hold that

Code: Select all

weight / 1000.0, # Weight, converted from grams to kilograms

is clearer than

Code: Select all

gram_to_kilogram_multiplier = 1.0 / 1000
weight * gram_to_kilogram_multiplier


The library I was using to output the CSV (tablib) complained if the rows in a Dataset were different lengths. This is reasonable if your data format is reasonable, but this is not the case. If I wanted to use tablib, I had to do:

Code: Select all

        for row in data:
            yield tablib.Dataset(row).csv


instead of the recommended-by-tablib

Code: Select all

yield tablib.Dataset(*data).csv

or similar things. Without comments, this code may have been "fixed" by a later developer to use th tablib recommended style.

For those who reject comments, what would have been your recommended soultion for creating this CSV? I really am interested, because I usually sit in the camp of writing self-documenting code, I just could not work out a reasonable way of doing it for this code. The full function can be found at: http://pastebin.com/66SK71kv

[EvanED]

While you could guess that

Code: Select all

weight / 1000.0

is a unit conversion, guessing in code is bad. Clever variable naming could have helped, but I hold that

Code: Select all

weight / 1000.0, # Weight, converted from grams to kilograms

is clearer than

Code: Select all

gram_to_kilogram_multiplier = 1.0 / 1000
weight * gram_to_kilogram_multiplier


while I'm definitely not one of those "you shouldn't use comments" people (and think that your second example seems like one of the archetype cases for where comments are really good, except that "output" is one word ), why not

Code: Select all

weight_kg = weight_g / 1000

?

(I do note that you have weight_in_grams in the actual code, but then just weight for the KG measure.)

[Yakk]

Code: Select all

weight_kg = weight_g / g_per_kg

or, rather:

Code: Select all

Units<int, grams> weight1 = whatever;
Units<int, kilo<grams>> weight2 = RoundUp(weight1);

ie, a type system that supports units and does conversion automatically (RoundUp is a function that returns a decorated input that, when type conversion happens, knows where to round. Using RoundUp in a non-type conversion situation generally generates a compile time error.

And when you introduce a new unit type (say, lbs, imperial_ton, and drams), you'd teach the code how to convert not in the main narrative of the code.

[Meem1029]

Does such a thing actually exist in any language? That actually sounds really awesome and useful

[thoughtfully]

There are plenty of physical unit classes around. I expect most OO languages have one or more, and the mainstream ones like Python and Java should have well maintained, well written ones. I know Scientific Python has em, and so do some other math/science libraries.

[Sagekilla]

Does such a thing actually exist in any language? That actually sounds really awesome and useful

F# actually has built in support for units. You can specify a unit system and it provides automatic conversion for you. So you
can say "This is in grams" then at a later point, say "I want this in kilograms" and it does it for you.

It's really easy to read code that makes use of the unit system in F#.

The Ordered product weight was in grams, but the CSV output needed to be in kilograms. Both fields were called 'weight'. While you could guess that

Code: Select all

weight / 1000.0

is a unit conversion, guessing in code is bad. Clever variable naming could have helped, but I hold that

Code: Select all

weight / 1000.0, # Weight, converted from grams to kilograms

is clearer than

Code: Select all

gram_to_kilogram_multiplier = 1.0 / 1000
weight * gram_to_kilogram_multiplier


I highly disagree with this. You have a magic number coming out of no where. I find it a million times easier to read something like:

const double GramsPerKilogram = 1000.0;

double weight_g = 1376;
double weight_kg = weight_g / GramsPerKilogram;

You explicitly state your intent without having to write anything extra. The code writes itself and explains itself. As a physicist, I also
like this approach better because the idea of unit conversion is very natural to me. The random division by 1000 seems very out of place
to me.

[sebwiers]

Had a conversation with a coworker today who said that you should never leave a comment explaining what code does--the function should be clear from reading the code; if it's not, you need to rewrite, not comment.
<SNIP>
Thoughts?

My thoughts are he's never worked in an IDE that made good use of comment based doc features (Eg, PHP doc blocs).
Why should I have to open the file containing the source code and read it, when (with proper commenting) I can just mouse_control on the function name and call up a tooltip explaining the functions purpose, parameters, and return values?

[undecim]

I think that the first half of that argument "the function should be clear from reading the code" is true, but you should still comment explaining the purpose of the function.

As a trivial example, consider this python example of the euclidean algorithm:

Code: Select all

def gcd(a,b):
        if b == 0:
                return a
        else:
                return gcd(b, (a%b))

It's clear WHAT the function does. Even the function name implies that it calculates the GCD of two numbers. It's also clear HOW the function does it, even if it's not immediately understandable to a person who has never seen this algorithm used.

So consider these two sets of comments:

Code: Select all

def gcd(a,b):
    # Calculate the GCD of two numbers
    if b == 0:
        #if b is 0, then return a.
        return a
    else:
        #otherwise, recurse.
        return gcd(b, (a%b))


These comments do nothing to clarify the code. (at least not to anyone who you want working on the code). If this is what your co-worker considers comments, then he's right. They're worthless. (and so is any code he wrote more than 1 year ago)

Now consider these comments:

Code: Select all

def gcd(a,b):
    # Calculate the Greatest Common Denominator of two numbers using the Euclidean Algorithm (see http://en.wikipedia.org/wiki/Euclidean_algorithm)
    # a and b are Numbers from which to calculate the GCD. They are interchangable
    if b == 0:
        return a
    else:
        return gcd(b, (a%b))


That comment is much more helpful. It explains the purpose of the function, the algorithm used, gives an external reference for more in-depth information, explains the purpose of each paramter, and even explains an important property of the function (i.e. that the parameters are interchangable.). For such a small function, they don't really seem so helpful, but more complex functions will benefit from these kinds of comments.

[sigsfried]

Comments certainly can be overused, but the idea of not using them at all for all but the most obvious of code seems silly. Just grabbing a very simple routine from my current code, I can see no way that it would be possible to be as clear without the comment, despite it being an exceptionally trivial bit of code.

Code: Select all

Subroutine get_S(Sx,Sy,Sz)
Double Precision, intent(out) :: Sz,Sy,Sx
!Returns the "classical" Sx,Sy,Sz using the Pauli Spin Matrices
Sx=2d0*(Rup*Rdown+Iup*Idown)
Sy=2d0*(Rup*Idown-Iup*Rdown)
Sz=(Rup**2+Iup**2)*2d0-1d0

End Subroutine


On the other hand I have seen some truly terrible commenting standards, for example my supervisors code has comments consisting purely of the date that line was changed.

[Xeio]

On the other hand I have seen some truly terrible commenting standards, for example my supervisors code has comments consisting purely of the date that line was changed.

Yea, I haven't seen anything quite that bad, though I have seen lots of comments with just a number for a bug in our tracker system. This is redundant because all our changesets are already marked with that number.

I've actually started deleting comments from the code base when they're useless as part of cleaning up the code. Or at least replacing them with a simple one line explanation rather than paragraphs that say the same thing...

[TheAmazingRando]

If you find yourself commenting excessively then it's a sign of bad code, and there's definitely something to be said for making your code as clear and self-documenting (well structured with descriptive variable names) as possible, but I don't think that makes comments themselves a bad thing.

Comments can be great for other people who are learning the system you're using, though. For example, take the Windows API pattern where you call the same function twice: once with a null output parameter to get the size you need, and again after you've allocated the appropriate space. If you've done a lot of Windows programming you'll recognize it instantly, and it isn't too difficult to figure it out, but a single comment explaining it would be useful. Granted, the Windows API is designed horribly so this kind of supports the idea that comments are a sign of bad design, but if you're working in the industry it's pretty much a given that you're going to encounter bad design and you're going to have to work with it and around it. Comments aren't necessary, but they can save time.

[EvanED]

If you find yourself commenting excessively then it's a sign of bad code, and there's definitely something to be said for making your code as clear and self-documenting (well structured with descriptive variable names) as possible, but I don't think that makes comments themselves a bad thing.

Comments can be great for other people who are learning the system you're using, though. For example, take the Windows API pattern where you call the same function twice: once with a null output parameter to get the size you need, and again after you've allocated the appropriate space. If you've done a lot of Windows programming you'll recognize it instantly, and it isn't too difficult to figure it out, but a single comment explaining it would be useful. Granted, the Windows API is designed horribly so this kind of supports the idea that comments are a sign of bad design, but if you're working in the industry it's pretty much a given that you're going to encounter bad design and you're going to have to work with it and around it. Comments aren't necessary, but they can save time.

I sort of agree with what you said, though the pedantic part of me has to point out that the "call once to get the size then again to get the data" is not really a Windows idiom at all; I mean, that's basically how you "have to" use snprintf.

I also don't think that the benefits of commenting an idiom like that makes sense if you've got more than a couple uses of it in your code base. Commenting lots of uses isn't worth it, and if you use it lots of places but only comment it a couple, chances are someone who is a newbie to your code base and the idiom will see one of the places it's not commented.

[Sagekilla]

If you find yourself commenting excessively then it's a sign of bad code, and there's definitely something to be said for making your code as clear and self-documenting (well structured with descriptive variable names) as possible, but I don't think that makes comments themselves a bad thing.

Comments can be great for other people who are learning the system you're using, though. For example, take the Windows API pattern where you call the same function twice: once with a null output parameter to get the size you need, and again after you've allocated the appropriate space. If you've done a lot of Windows programming you'll recognize it instantly, and it isn't too difficult to figure it out, but a single comment explaining it would be useful. Granted, the Windows API is designed horribly so this kind of supports the idea that comments are a sign of bad design, but if you're working in the industry it's pretty much a given that you're going to encounter bad design and you're going to have to work with it and around it. Comments aren't necessary, but they can save time.

I sort of disagree with this. If you find yourself doing something like:

Code: Select all

// Call foo twice, once to get size, another to get the data
int size = Foo(NULL);
char *buffer = new char[size];
Foo(buffer);


What you should really be doing is wrapping that call:

Code: Select all

char *GetTheFoo(int &size)
{
    size = Foo(NULL);
    char *buffer = new char[size];
    Foo(buffer);
    return buffer;
}


Of course, now you run into issues of having the caller having to remember to deallocate the buffer.
But in general, you should design in a way that makes things easier to do.

[Yakk]

Or, rather:

Code: Select all

std::vector<char> GetTheFoo()
{
  // Calling Foo with a null returns the size.
  int size = Foo(nullptr);
  std::vector<char> retval;
  retval.resize(size);
  int size2 = Foo( &retval.front() );
  Assert(size == size2);
  return std::move(retval);
}

and now the caller doesn't have to deallocate the buffer. Plus, the caller is told how big the buffer is.

In more cases, Foo will take a buffer and a length, and if it is insufficient return the length it needs. Those can be extended with a GetTheFoo() that takes an optional length guess, and if that guess is right returns it in one go.