What is an adequate amount of commentary on code?

Fenster

I mean, how much is too much, or too little? Every time I look at someone else's code, or come back to my own code after a long break, all I see is a glob of random characters that I have a difficult time parsing, and so for my own sake I tend to be verbose in how I comment.

Is this bad practice? Annoyingly much? Sure, it will vary by workplace, but as a rule is this fine?

Example function:

function addLightbox(obj) {
    // Append empty lightbox to whatever.
    // Best prepended to body.
    var divOpen = '<div class="';
    var divClose = '"></div>';

    var lbElements = [
        boxName + '-nav',
        boxName + '-txt',
        boxName + '-img',
        boxName + '-close'
    ];

    // Append lightbox to foo.
    $(obj).prepend(divOpen + boxName + '">');
    // Insert other elements.
    $(lbElements).each(function(i,e) {
        $('.' + boxName).prepend(divOpen + e + divClose);
    });

    // Insert nav elements.
    $('.' + lbElements[0]).append(divOpen + lbElements[0] + '-left' + divClose);
    $('.' + lbElements[0]).append(divOpen + lbElements[0] + '-right' + divClose);

    // Lightbox height.
    changeObjHeight('.' + boxName, $(window).height());

    // Blurb position.
    $('.' + lbElements[1]).css('margin-top', $(window).height() * 0.85);
}

oscarBravo

That looks pretty good to me. I tend to be inspired by Python's PEP8 guide to inline comments:

...comments are unnecessary and in fact distracting if they state the obvious. Don't do this:

x = x + 1                 # Increment x

But sometimes, this is useful:

x = x + 1                 # Compensate for border

Ziycon

However much is needed to clearly and concisely explain what's happening also no need to comment general code that any programmer should understand or use on a daily basis like assignments, basic calculations, loops etc unless there doing something that needs to be noted. Your commenting above looks grand.

colm_c

As mentioned previously, it's best to describe what the code does, not how it does it, as if you can't understand what's happening in the program then it's not very well written IMO.

Also naming variables appropriately solves some of the readability issues IMO.

e.g.

var x = 100

vs

var lightboxHeight = 100

Is always easier to read back and understand what's what.

If you're paranoid (especially with client side programming, like js) you can always run i through an obfuscater.

Also, not sure if that code below is real, but there's a bunch of optimisations that could be easily applied to make it much easier to read and understand 6 months later.

Tar.Aldarion

If you named the variable LightBox You wouldn't need the likes of // Append lightbox to foo.
I am a big fan of naming variables descriptively.

Calina

Looks okay to me.

I'm a fan of documenting the why and again, using appropriate variable names.

Colonel Panic

I only use comments to apologise to future maintainers of my code.

oscarBravo

Colonel Panic wrote: »

I only use comments to apologise to future maintainers of my code.

Ah, the "abandon all hope..." school of comments.

Tar.Aldarion

Here thar be obfuscation

Sparks

The golden standard is Can another average programmer understand your code by reading it (and "Future You" is another average programmer for the purposes of the test).

If yes, sufficient comments are in place. If no, then the other programmer is going to curse your name from here to whenever he or she is asked to give you a job recommendation in our tiny little industry.

(Don't you love how source control means bad code is no longer anonymous?)

Personally, I prefer people to err on the excessive side when commenting. It requires one keystroke to delete a superfluous comment line. It requires rather more to write an unwritten comment line when we find out a year later that it's badly needed...

GreeBo

oscarBravo wrote: »

Ah, the "abandon all hope..." school of comments.

// should never get here!


Comment things that are not immediately obvious why they are happening or what is happening.

Variable names and method names go a long way to this.

Sparks

GreeBo wrote: »

// should never get here!

Thank dog, I thought it was just me who'd run into that gem...

Tar.Aldarion

I've seen should never get here comments in blizzard games. Heh.

Svn blame is aptly named

Sparks

Don't even have to go that far http://www.boards.ie/vbulletin/showpost.php?p=66235590&postcount=43

IRLConor

GreeBo wrote: »

// should never get here!

Gah. Hate that.

If you don't think it's reachable, then put your money where your mouth is and abort()/throw/raise/whatever.

Colonel Panic

IRLConor wrote: »

Gah. Hate that.

If you don't think it's reachable, then put your money where your mouth is and abort()/throw/raise/whatever.

catch
{
   // Pokemon!
}

Sorted.

Manach

Try to use perhaps latin or Irish comments.
The former has a conciseness and when I used the latter, there was an interesting discussion on language rights during the code review

Stark

I tend to be on the side of less is more. If you need comments to explain your code then your code needs to be refactored. Try using descriptive variable names and breaking out your code into smaller descriptively named functions.

Problems with comments include adding code bloat (more scrolling to see what's going on) and comments not being updated when code is updated. Code errors will be caught out by compiler, unit tests, bugs etc. Inaccurate comments tend to linger and add noise.

Sparks

Stark wrote: »

Problems with comments include adding code bloat

Yeah, I'll take that all day, all night and twice on tuesdays, sideways, if the alternative is sitting there scratching my head at a chunk of code that makes no sense, several years after the guy who wrote it quit, with a deadline to debug it and a customer yelling down the phone about it.

BrokenArrows

Don't comment obvious code with what the code is doing. However, if the reason for that bit of code is unclear (And you cannot make it clear) comment WHY you are doing it.

The Corinthian

If code has been clearly written, well indented and variable names are meaningful, you actually should not need any commenting as a competant developer should be able to read and understand it with ease.

If you do comment the code, it should add to one's understanding of it, such as sign-posting different parts to say what is being handled there, e.g.

// User log-out handled
...

// Validation of user input
...

Or briefly describing what a method/function does and perhaps it's params/return value (if not straightforward, e.g. an array in a specific format is returned).

But this would me minimal commenting, which would be my preference, as I find excessive commenting superfluous.

Tar.Aldarion

The Corinthian wrote: »

If code has been clearly written, well indented and variable names are meaningful, you actually should not need any commenting as a competant developer should be able to read and understand it with ease.

Sparks

The Corinthian wrote: »

If code has been clearly written, well indented and variable names are meaningful, you actually should not need any commenting as a competant developer should be able to read and understand it with ease.

Ten years in industry, seven in academia, and I can count the number of lines of such code that I've seen on the fingers of one hand. While making a rude gesture with it. (I'd love to say all my code qualifies as this, naturally, but apart from a lack of people looking to beat my head in with a keyboard, I've no proof of that).

(I'm not counting example code in books for this or reference code written to illustrate a point, I'm talking actual working code from real projects here)

Frankly, I've had days on projects where if I got ten lines of comment per line of code, I'd fall to my knees singing the praises of flowers, trees and legal narcotics, regardless of the overcommenting because compared to undercommenting, overcommenting is like having too much money in your bank account...

ChRoMe

The Corinthian wrote: »

If code has been clearly written, well indented and variable names are meaningful, you actually should not need any commenting as a competant developer should be able to read and understand it with ease.

In your bog standard CRUD apps maybe, complex algorithms, not a hope.

Fenster

Thank you everyone for your answers. The gist that I take away is that my commentary is fine, other programmers have the same problem with even their own code being illegible after a break, but I should continue to be wary of excessive remarks on elementary lines.

moycullen14

Came across one recently that made me wonder about the efficacy of commenting in general...

It was a complicated java/hibernate criteria with projections, multi-table joins, aggregation, the whole nine yards. This stuff is hard to read at the best of times but this was way beyond normal.

The developer had - thankfully - included included the generated SQL as a comment. This was a big help to understanding what he was trying to do. Unfortunately the comment was wrong. Usually you'd think that this is worse than no comment at all but because I could see what he was trying to do from the comment, it helped me understand the code.

With modern API-driven development, the structure of the code is usually obvious. Where you want comments is where things are done in non-standard ways or where it is sufficiently complicated to warrant some explanation.

Integration with SCM is key too. If you have to write an essay on a piece of code - and sometimes you do - tag the code in the SCM and the reader can access the description without cluttering up the code.

This one made me smile

Exception up = new Exception("Something is really wrong.");
throw up;  //ha ha

c_man

I'm of the minimalist school of thought when it comes to commenting. I think in general if you find yourself putting a lot of comments in* then something is wrong be it the code, design or just your own approach.

Sign, I'm muddling through a heavily commented module now on a bug fix. The vast majority of them are inaccurate.



*excluding the likes of API headers and other common sense scenarios you'll throw at me.

fergalr

Everything that makes your source bigger is something extra to maintain - so every comment has a (notional) cost.

So, basically, there's a point at which the cost of the comment outweighs the value it brings through increased clarity. At that point, don't add the comment.

Where that point is depends on what you are writing, who you are writing for, lifetime of the project, how likely to comments are to be maintained etc.

There's no one point answer, but the distribution of answers in this thread is pretty good I'd say.

Sparks

fergalr wrote: »

Everything that makes your source bigger is something extra to maintain - so every comment has a (notional) cost.

Of course, that argument doesn't have an actual answer buried in it, just a metric

For example, unit tests add to the size of your codebase too, and there's research showing that code review is more effective than unit tests for finding bugs (in terms of how many it finds). So do we stop writing tests for code?

fergalr

Sparks wrote: »

Of course, that argument doesn't have an actual answer buried in it, just a metric

For example, unit tests add to the size of your codebase too, and there's research showing that code review is more effective than unit tests for finding bugs (in terms of how many it finds). So do we stop writing tests for code?

When the cost of maintaining the test exceeds the value it brings in terms of code correctness, and ability to make changes with confidence, etc. then the test should be not written.

So, yes, basically.

There's a cost to writing each test, and to having each test sitting in the codebase, and to every time a programmer thinks "I could refactor that, but then I'd have to update all my tests."

Complexity is one of our main enemies. Every framework, tool, scaffolding, test, harness, comment, and SLOC has a cost in terms of complexity. Each such component of the system must either provide benefit exceeding its cost, or be deleted.

Where that balance point is will depend on a bunch of factors, but we should remember that we're always trying to strike that balance.
There's definitely a 'test usefulness' threshold, below which we just shouldn't write that test.

I know my estimate of that threshold seems to be much higher than a lot of people involved in, say, TDD.

GreeBo

fergalr wrote: »

When the cost of maintaining the test exceeds the value it brings in terms of code correctness, and ability to make changes with confidence, etc. then the test should be not written.

So, yes, basically.

There's a cost to writing each test, and to having each test sitting in the codebase, and to every time a programmer thinks "I could refactor that, but then I'd have to update all my tests."

Complexity is one of our main enemies. Every framework, tool, scaffolding, test, harness, comment, and SLOC has a cost in terms of complexity. Each such component of the system must either provide benefit exceeding its cost, or be deleted.

Where that balance point is will depend on a bunch of factors, but we should remember that we're always trying to strike that balance.
There's definitely a 'test usefulness' threshold, below which we just shouldn't write that test.

I know my estimate of that threshold seems to be much higher than a lot of people involved in, say, TDD.

Wouldnt a test thats difficult to maintain imply that the code its testing is also difficult to maintain....and having a test is thus a lovely warm fuzzy blanket?

fergalr

GreeBo wrote: »

Wouldnt a test thats difficult to maintain imply that the code its testing is also difficult to maintain....and having a test is thus a lovely warm fuzzy blanket?

Not necessarily. I can imagine valid situations where your test code is complex (and maybe even necessarily so) although the code its testing is simple.

Maybe we are talking about the tests for your pseudo-random number generator.


Or maybe someone has decided they want to test your code is properly generating the mandelbrot set and come up with a very complex test for some fairly simple code.


But anyway, lets say we accept your premise - what's the relevance really?

There's often a benefit to having tests, sure; that's not in dispute; I'm just saying there's a complexity cost too, and the two have to be compared.

GreeBo

fergalr wrote: »

But anyway, lets say we accept your premise - what's the relevance really?

There's often a benefit to having tests, sure; that's not in dispute; I'm just saying there's a complexity cost too, and the two have to be compared.

The relevance is that if I have complex code that is being changed, I want a confidence level that nothing unexpected/unnoticed has been broken.

Unit tests give you this confidence level. I think there is always a benefit to having unit tests, sure there may be a cost, but there is a cost with any code you write, error handling for example, logging another, but we accept that cost because the benefits outweigh it.

moycullen14

Here's a good comment.

. . .

hashOut.data = hashes + SSL_MD5_DIGEST_LEN;
hashOut.length = SSL_SHA1_DIGEST_LEN;
if ((err = SSLFreeBuffer(&hashCtx)) != 0)
    goto fail;
if ((err = ReadyHash(&SSLHashSHA1, &hashCtx)) != 0)
    goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &clientRandom)) != 0)
    goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0)
    goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
    goto fail;
[COLOR="Red"]    goto fail;  /* MISTAKE! THIS LINE SHOULD NOT BE HERE */
[/COLOR]if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)
    goto fail;

err = sslRawVerify(...);

From the recent iOS security upgrade

http://nakedsecurity.sophos.com/2014/02/24/anatomy-of-a-goto-fail-apples-ssl-bug-explained-plus-an-unofficial-patch/

Sparks

GreeBo wrote: »

I think there is always a benefit to having unit tests

Without disagreeing or agreeing and in all seriousness, where's the study that proves that?
(See here for the motivation to the question).

Sparks

moycullen14 wrote: »

Here's a good comment.
...
From the recent iOS security upgrade

Is it just me, or wouldn't even lint have caught that?

GreeBo

Sparks wrote: »

Without disagreeing or agreeing and in all seriousness, where's the study that proves that?
(See here for the motivation to the question).

Probably the same place as the study that disproves it?

However, I think of it a lot like insurance, it doesnt have to be useful every day/ time to be worth it.
Plenty of times I've confidently made changes only to see the tests fail and go "oh yeah....that!"

Like a helmet or seatbelt, once is enough for me.

Sparks

GreeBo wrote: »

Probably the same place as the study that disproves it?

Either study would be good, but that's kindof sidestepping the point by a country mile...

However, I think of it a lot like insurance, it doesnt have to be useful every day/ time to be worth it.

And like insurance, sometimes there's absolutely no point in having it (eg. extended warranties which don't exceed the EU minimum legal basics, insuring things you can afford to replace, etc, etc).

But unlike insurance, this is something we really ought to prove. Just as a basic industry practice, y'know? For example, what if the study shows that there is a benefit, but it's only worth it once $METRIC exceeds $THRESHOLDVALUE ?

IRLConor

Sparks wrote: »

Without disagreeing or agreeing and in all seriousness, where's the study that proves that?
(See here for the motivation to the question).

"Making Software, What Really Works and Why We Believe It" has a nice large collection of references to good studies around software development practices.

It doesn't have studies specifically on unit testing but it does include a systematic review on TDD which is quite interesting.

Calina

Ooh there's a kindle edition of that. I might buy it and add it to the other books I have to read....

IRLConor

Calina wrote: »

Ooh there's a kindle edition of that. I might buy it and add it to the other books I have to read....

Yup, I have the Kindle edition. It's the only way I was able to check it for testing references since I'm not currently co-located with my bookshelf.

Sparks

IRLConor wrote: »

"Making Software, What Really Works and Why We Believe It" has a nice large collection of references to good studies around software development practices.

And is my current book-in-progress on the work reading list
(Though ooo, kindle - that might help speed up my progress on it...)

It doesn't have studies specifically on unit testing but it does include a systematic review on TDD which is quite interesting.

Yup, but the unit testing thing is more general than TDD, and it'd be nice to see if someone ever studied - for example - whether the benefits of unit testing don't kick in until after the initial development (Micheal Feather's book on legacy systems describing the kind of thing I'm thinking about here), whether or not code review was better than unit tests in the beginning (there's some work on code review, but not that specific question), or whether you get the benefits right from day one.

That's kindof the thing really - we all know that it's so, the same way people always knew a whole bunch of things that later scientific studies showed were completely bogus. So actual evidence is downright necessary if we want to be able to say our profession is just another kind of engineering (in the generic applying-science-and-economics-to-build-the-world-around-us sense of the word).

Calina

now that I am actually thinking about this, I wonder how much of this is driven by the IT process consultancy industry.

IRLConor

Calina wrote: »

now that I am actually thinking about this, I wonder how much of this is driven by the IT process consultancy industry.

Probably less than the amount that's driven by hype from loudmouth self-publicists.

GreeBo

Sparks wrote: »

Either study would be good, but that's kindof sidestepping the point by a country mile...

tbf, its sidestepping it just as much as you are. If there is no empirical evidence either way, then the "evidence" argument is moot, no?

I can't prove that unit tests are always beneficial any more than you can prove they aren't.

Sparks wrote: »

And like insurance, sometimes there's absolutely no point in having it (eg. extended warranties which don't exceed the EU minimum legal basics, insuring things you can afford to replace, etc, etc).

Whats the equivalent to free insurance in software development though?
I think we can all agree that the earlier you find a bug the cheaper it is to fix? Can any industry really afford (or would want?) to fix bugs that are not found during development/construction?

Sparks wrote: »

But unlike insurance, this is something we really ought to prove. Just as a basic industry practice, y'know? For example, what if the study shows that there is a benefit, but it's only worth it once $METRIC exceeds $THRESHOLDVALUE ?

I dont believe you can prove it any more than the source control point. Whats the baseline; your company, mine, some other, Google?

For me the bottom line is that if you are paying people to write code that you want to sell/make money from, then you want them doing that as much as possible. Taking the hit of having them write unit tests means that any bugs introduced are typically fixed by the creator at a much cheaper cost than being found by another person and potentially fixed by yet another person.

A test is written once and will continue "proving" the code still works as intended until the code is modified to change that expected behaviour. Thats a lot of free regression testing that you wont get with manual testing/code reviews.

Sparks

GreeBo wrote: »

tbf, its sidestepping it just as much as you are. If there is no empirical evidence either way, then the "evidence" argument is moot, no?

I don't see how -- the argument is that we should collect evidence to find out. It's not an argument for or against the method, just that we should find out first.

I can't prove that unit tests are always beneficial any more than you can prove they aren't.

That's not the argument I'm making. I'm specifically saying I neither disagree nor agree with you. I literally don't know either way. I can see good arguments for both sides of that and some for other sides too (yes, we're up to N sides here, bear with me).

Whats the equivalent to free insurance in software development though?

There's no such thing either in the insurance or the software world (unit tests aren't free). My analogy was trying to say (badly) that with insurance you know what it costs and you have guidelines based ultimately on maths that tell you if you need insurance or not. We don't seem to have those guidelines for unit tests because we don't appear to have the underlying data.

I think we can all agree that the earlier you find a bug the cheaper it is to fix?

Well, yes as it happens because someone studied that and proved it.
But there's more than one way to find bugs. And what little actual experimental evidence we have has shown that at least one of those other methods is highly effective in the right circumstances (code review). And there's nothing to say you have to choose one or the other either, nobody's studied that approach.

A test is written once and will continue "proving" the code still works as intended until the code is modified to change that expected behaviour.

That's a good argument.
"So long as the test was right in the first place" is a good counterargument (and one I've hit in practice on code that's been shipping for a long time).
And when you have good arguments for and against, it really highlights that we need the problem to be studied more. That's my point here. We don't have those studies yet and we ought to have them.

GreeBo

Sparks wrote: »

I don't see how -- the argument is that we should collect evidence to find out. It's not an argument for or against the method, just that we should find out first.

That's not the argument I'm making. I'm specifically saying I neither disagree nor agree with you. I literally don't know either way. I can see good arguments for both sides of that and some for other sides too (yes, we're up to N sides here, bear with me).




There's no such thing either in the insurance or the software world (unit tests aren't free). My analogy was trying to say (badly) that with insurance you know what it costs and you have guidelines based ultimately on maths that tell you if you need insurance or not. We don't seem to have those guidelines for unit tests because we don't appear to have the underlying data.


Well, yes as it happens because someone studied that and proved it.
But there's more than one way to find bugs. And what little actual experimental evidence we have has shown that at least one of those other methods is highly effective in the right circumstances (code review). And there's nothing to say you have to choose one or the other either, nobody's studied that approach.


That's a good argument.
"So long as the test was right in the first place" is a good counterargument (and one I've hit in practice on code that's been shipping for a long time).
And when you have good arguments for and against, it really highlights that we need the problem to be studied more. That's my point here. We don't have those studies yet and we ought to have them.

And the arguement makes sense, but and there is always a but, I'm going to continue demanding unit tests until someone proves to me that they cost more than they are worth.

Code reviews should help you to have correct tests

Sparks

GreeBo wrote: »

And the arguement makes sense, but and there is always a but, I'm going to continue demanding unit tests until someone proves to me that they cost more than they are worth.

That's as valid as anything else given what we know at the moment.

Code reviews should help you to have correct tests

Yes; but again, the counterargument is that code reviews and unit tests aren't free and if code reviews catch over 90% of all bugs before you ever even run the compiler the first time, would we be better off spending time on those and not on unit tests?
(And no, we don't know the answer. Could be either, could be both).

moycullen14

Sparks wrote: »

That's as valid as anything else given what we know at the moment.

Yes; but again, the counterargument is that code reviews and unit tests aren't free and if code reviews catch over 90% of all bugs before you ever even run the compiler the first time, would we be better off spending time on those and not on unit tests?
(And no, we don't know the answer. Could be either, could be both).

Am I right in saying that code reviews is the one thing that has been empirically proven to improve quality/lower cost in development? You rarely come across it, though. I guess because if (when) it is badly managed, it causes a lot of problems.

Calina

moycullen14 wrote: »

Am I right in saying that code reviews is the one thing that has been empirically proven to improve quality/lower cost in development? You rarely come across it, though. I guess because if (when) it is badly managed, it causes a lot of problems.

Was mandatory at my last job. Nothing went to UAT without code review. I suppose this is why I get stunned when I hear that it's not done in a lot of places.

moycullen14

Calina wrote: »

Was mandatory at my last job. Nothing went to UAT without code review. I suppose this is why I get stunned when I hear that it's not done in a lot of places.

It's just that in a long(ish) career here and in the UK, I have rarely come across it being done systematically. One of the few places I saw it had terrible problems
with it. Temper tantrums, dysfunctional behavior, the lot. It was down to a lack of coding standards, mainly, so everything was based on prejudice.

Maybe with API & Framework based systems, it should be less contentious.

One thing for sure, most environments would benefit hugely from code walk-throughs - just as long as I don't have to do it!