Code Comment Best Practice

Gurgle

Skrynesaver wrote: »

I'd read the code in the method in question rather than the comment that said it was wonderful and shat rainbows

Generally if you're in there actually reading the fine detail of someone else's code (or your own code from a year ago) its because it doesn't work as intended.

Read the comment to see what it was intended to do, then read the code to see what it actually does.

-edit- oh yeah, comments above code

Anima

Gurgle wrote: »

Read the comment to see what it was intended to do, then read the code to see what it actually does.

Who is to say that the comment is correct?

BrokenArrows

Commenting should be explaining WHY your code does something rather than what it does. People should be able to workout what something does.

An Example would be a FOR loop staring at 2 instead of 0.
Not much point in commenting:
// loop staring at 2 to 100

Needs to be:
// loop starting at 2 because array locations 0 and 1 are place holders.

Not a person

When starting this thread i could never have anticipated the amount of comments it would generate.

Interesting and differing views.

Elfman

I think after a while you don't need to think about this but as a rule of thumb.

- If the comment relates to a line //Do it to the right

//You should do it above
If it relates to a block of code or will take multiple comment lines
line this

Smoggy

BrokenArrows wrote: »

Commenting should be explaining WHY your code does something rather than what it does. People should be able to workout what something does.

An Example would be a FOR loop staring at 2 instead of 0.
Not much point in commenting:
// loop staring at 2 to 100

Needs to be:
// loop starting at 2 because array locations 0 and 1 are place holders.

Could better named variables negate the need for comments in your example ?


iPostPlaceHolderArrayStart = 2
iArrayEnd = 100

for(int i=iPostPlaceHolderArrayStart; i<iArrayEnd; i++)
{
// Test
}

GreeBo

Smoggy wrote: »

Could better named variables negate the need for comments in your example ?


iPostPlaceHolderArrayStart = 2
iArrayEnd = 100

for(int i=iPostPlaceHolderArrayStart; i<iArrayEnd; i++)
{
// Test
}

In this (albeit contrived) example I think comments would be far easier to understand, sure you could also use more descriptive names, especially if its a nested loop for example (bah use recursion! )

PrzemoF

Smoggy wrote: »

Could better named variables negate the need for comments in your example ?


iPostPlaceHolderArrayStart = 2
iArrayEnd = 100

for(int i=iPostPlaceHolderArrayStart; i<iArrayEnd; i++)
{
// Test
}

Those "placeholders" smell like a nasty workaround... Ooops, off-topic!

Smoggy

PrzemoF wrote: »

Those "placeholders" smell like a nasty workaround... Ooops, off-topic!

There is no doubt the first two positions of the array being used as a place holder is bad, But we will ignore that.

How about this, that when the code needs to get updated, lets say another developer uses array position 2 as a place holder. In this case the code AND the comment needs to be updated.

More often than not the code gets updated, but the comments left or someone moves this block of code furthur up the function, but left the comments in situ. In my world this isn't a situation. No comments, no need to update them.

In my example :

iPostPlaceHolderArrayStart = 3
iArrayEnd = 100

for(int i=iPostPlaceHolderArrayStart; i<iArrayEnd; i++)
{
// Test
}

Colonel Panic

I've seen arrays with the size of the array in the first element...

GreeBo

Smoggy wrote: »

There is no doubt the first two positions of the array being used as a place holder is bad, But we will ignore that.

How about this, that when the code needs to get updated, lets say another developer uses array position 2 as a place holder. In this case the code AND the comment needs to be updated.

More often than not the code gets updated, but the comments left or someone moves this block of code furthur up the function, but left the comments in situ. In my world this isn't a situation. No comments, no need to update them.

In my example :

iPostPlaceHolderArrayStart = 3
iArrayEnd = 100

for(int i=iPostPlaceHolderArrayStart; i<iArrayEnd; i++)
{
// Test
}

So your argument is to avoid the case where someone needs to read the code because the comment is out of date, you want someone to have to read the code everytime?!

Presumably the person who changed the code had to read the comment to figure out what to change, if they then dont update the comment to reflect the change they are a goon, and poor comments are probably the least of your worries with their code.

BrokenArrows

Lol..I love the hatred of the placeholder example. It was just some random thing I thought of on the spot that would require some explanation.

Smoggy

GreeBo wrote: »

So your argument is to avoid the case where someone needs to read the code because the comment is out of date, you want someone to have to read the code everytime?!

Presumably the person who changed the code had to read the comment to figure out what to change, if they then dont update the comment to reflect the change they are a goon, and poor comments are probably the least of your worries with their code.

In the original example you were going to have to read the comment and the code, my point is well written code is easy to read and makes comments redundant in many cases.

The example of updating the code and not the comment is extreme, but highlights how they are needed to be kept in sync and together when I suggesting that you don't need comments.

Just read the code, job done.

conor.hogan.2

Creamy Goodness wrote: »

If the comments are good I won't need to read the code, therefore the comment should come first.

Comments can and often do "lie" though.

BrokenArrows

conor.hogan.2 wrote: »

Comments can and often do "lie" though.

True, but you can also have code that makes perfect sense but not be doing what was intended.

conor.hogan.2

BrokenArrows wrote: »

True, but you can also have code that makes perfect sense but not be doing what was intended.

Of course, but the code is always doing what is done. Whether that is right or not is another thing. But what the code says, the code does.

Smoggy

BrokenArrows wrote: »

True, but you can also have code that makes perfect sense but not be doing what was intended.

That's where your automated unit tests come in, the original developer who wrote the code, also wrote the unit tests. Therefore keeping the code always in aligment with what it was intended to do.

amen

That's where your automated unit tests come in, the original developer who wrote the code, also wrote the unit tests. Therefore keeping the code always in aligment with what it was intended to do

but both the code and the unit tests might agree but the code could be still be wrong in that it is not doing what its meant to.

Feathers

GreeBo wrote: »

Again, its counter productive to have everyone read and understand every bit of code they interact with. Do you read your way down through every part of java.io package before you read from a file, or do you read the appropriate javadoc/api?

Conversely, do you read all the way through the JavaDocs of IO to find the method you want to use?

Generally, I'm coming at it from the other direction - if I know roughly what a class is doing, I know roughly the operation that I want to do on its object & look for a method signature to match. You only need comments here if the method signature is confusing or has side effects.

One good example is the Joda DateTime class - if I trust that its plus and minus methods work in a sensible way & are thread safe, I can happily call plusMonth(1) just from the method name. If I don't trust that it will handle year-ends correctly, I'd have to dig into the code anyway, even if I had a comment.

IMO, I'd go for comments to show the why rather than the what - if you can't read what it's doing, I'd split the inner workings into smaller private methods. For things like deprecation though, you can't tell be reading the annotation why the use of the method should be phased out.

Even most other 'why' comments can sit happily in VCS commit logs too, where you can have his issue number (why he made the change), his summary of changes & a reason for the approach & a diff against the last revision.

Not a person wrote: »

When starting this thread i could never have anticipated the amount of comments it would generate.

& they were generated neither to the right nor above the original post :P

GreeBo

amen wrote: »

but both the code and the unit tests might agree but the code could be still be wrong in that it is not doing what its meant to.

If the tests are passing then you have to assume the code is doing what the developer intended, that may not tie in with the requirements, but thats a whole different argument.

Feathers wrote: »

Conversely, do you read all the way through the JavaDocs of IO to find the method you want to use?

No, I read the exposed interface and assume that the rest of it works as intended, most often i dont care about the details of the inner workings unless something is not working as I expected.

Feathers wrote: »

Generally, I'm coming at it from the other direction - if I know roughly what a class is doing, I know roughly the operation that I want to do on its object & look for a method signature to match. You only need comments here if the method signature is confusing or has side effects.

One good example is the Joda DateTime class - if I trust that its plus and minus methods work in a sensible way & are thread safe, I can happily call plusMonth(1) just from the method name. If I don't trust that it will handle year-ends correctly, I'd have to dig into the code anyway, even if I had a comment.

But I dont think anyone is arguing with you about this, no one is saying comment everything, rather comment the bits that would benefit from an explanation, should someone want to read it.

Feathers wrote: »

IMO, I'd go for comments to show the why rather than the what - if you can't read what it's doing, I'd split the inner workings into smaller private methods. For things like deprecation though, you can't tell be reading the annotation why the use of the method should be phased out.

Even most other 'why' comments can sit happily in VCS commit logs too, where you can have his issue number (why he made the change), his summary of changes & a reason for the approach & a diff against the last revision.

Yes you should always explain what/why you made changes in your commits but I dont want to have to trawl through commits to find out the basic reason for something, only if I need to track it back down to a requirement change/defect etc.

Smoggy

amen wrote: »

but both the code and the unit tests might agree but the code could be still be wrong in that it is not doing what its meant to.

So we are saying the code and the unit tests are wrong, yet the comment is right ? This situation has to be few and far between ?

satchmo

Not commenting code is just lazy, pure and simple. There's no other excuse - writing code takes time, writing comments doesn't. And if I'm working on code that interacts with yours and need to review it, I want to read a comment instead of having to pause to grep your code, no matter how self-documenting it is. Because your idea of self-documenting isn't necessarily the same as mine.

But as already mentioned earlier in the thread, the best reason to comment code is for yourself six months down the line. Many things are obvious and apparent when you're in the middle of solving a problem, but after time and thousands of lines of other code, that isn't still the case.

Tar.Aldarion

Yeah for the example there was a problem with hardware design of a product I was coding on recently and therefore I had to follow a weird unintuitive procedure in the registers to get the functionality working as intended. Thank god I had it commented as a month later I had no idea what was going on without the comment, especially useful as other people had to edit it and understand it.

GreeBo

^ another good example of why commenting is good.
Ok, unless the code is poorly written most people will be able to understand it by spending some time reading it and maybe debugging. But this is a terrible waste of resources if everyone who touches the code has to do it each time. It takes a couple of mins to write and read a comment.

mewso

satchmo wrote: »

Not commenting code is just lazy, pure and simple. There's no other excuse - writing code takes time, writing comments doesn't. And if I'm working on code that interacts with yours and need to review it, I want to read a comment instead of having to pause to grep your code, no matter how self-documenting it is. Because your idea of self-documenting isn't necessarily the same as mine.

But as already mentioned earlier in the thread, the best reason to comment code is for yourself six months down the line. Many things are obvious and apparent when you're in the middle of solving a problem, but after time and thousands of lines of other code, that isn't still the case.

Sorry but I'm with the "keep comments to a minimum" group on this. Poisoned by Uncle Bob I suppose. Are you saying a function called AddTwoNumbersTogether equates to "your idea of self-documenting isn't necessarily the same as mine". It's simple really. Comment code that genuinely requires it and keep it to a minimum. Well written, clean code should not require comments. The more comments I see the more I will expect to see meaningless function and variable names. Whenever I see people associating laziness with not commenting I'm thinking their code will be an obfuscated mess because they have the crutch of comments to get them off the hook and use an x here, a y there and a DoProcessA name all over the place.

Duggys Housemate

Yes. Besides the documentation commenting each function there should be no need to comment unless the code has to do something odd.

I don't like this.

switch(y){ // position in race
case 0: // first
.....
break;

case 1: // second
,......
break;
}

etc.

Enums are your friend, and take more time

satchmo

mewso wrote: »

Sorry but I'm with the "keep comments to a minimum" group on this. Poisoned by Uncle Bob I suppose. Are you saying a function called AddTwoNumbersTogether equates to "your idea of self-documenting isn't necessarily the same as mine". It's simple really. Comment code that genuinely requires it and keep it to a minimum. Well written, clean code should not require comments.

Well that's just a silly example. But if you want to use that, I would argue that this code:

// Add two numbers together
int Add(int& x, int& y)
{
    return x+y;
}

is clearer and easier to glance at than this code:

int AddTwoNumbersTogether(int& firstNumber, int& secondNumber)
{
    int result = firstNumber + secondNumber;
    return result;
}

But again, this is a silly example. Most code isn't this simple, and you quickly get to the point at which a few lines of comments are much easier and quicker to understand than reading through many lines of code, no matter how explicit it is.

The more comments I see the more I will expect to see meaningless function and variable names. Whenever I see people associating laziness with not commenting I'm thinking their code will be an obfuscated mess because they have the crutch of comments to get them off the hook and use an x here, a y there and a DoProcessA name all over the place.

Don't confuse code quality and number of comments, they have little to do with each other. Bad code is bad code, whether it's commented or not.

My main point is that I've seen plenty of uncommented 'self-documenting' code where it takes far too long to figure out what the code is actually doing. So I could also say that whenever I see people advocating self-documenting code over comments, I'm thinking their code is going to be an obfuscated mess and I'm going to have to stop whatever work I'm doing to try and figure out what their code actually does. A few comments means I can have a quick glance, and then continue with my own work.

Duggys Housemate

Satchmo, your comment is a documentation comment describing the function. Nobody is arguing against that.

Comments in the code, within the body, waste space. If you top comment every line you will double the size of the code. Comments should be to explain difficult or unusual circumstances, and using them instead of stronger typing is a cop out.

If you are doing a switch statement which compares values which are in fact ints, the easiest way is to comment the cases, the harder, safer, but better way is to typedef an enum list to a value - in my example you would typedef to a PositionInRace, with the individual euums being kRacePositionFirst etc. Not only is the code more readable the compiler will warn if you miss a case.

Tar.Aldarion

Duggys Housemate wrote: »

Satchmo, your comment is a documentation comment describing the function. Nobody is arguing against that.

I thought people were, saying the method name and variable names should be there instead. or else the method should be refactoring due to being too complext and so on?

GreeBo

Duggys Housemate wrote: »

Comments in the code, within the body, waste space. If you top comment every line you will double the size of the code.

Are you printing out code or something?
Who cares if the source code is large because it has comments in it...am I totally missing your point here? Whats the issue?