Commenting your code - what, when, where and how?

Procasinator

Sparks wrote: »

printme &= IIf(i Mod 3 = 0, "FIZZ", "")

I'd have a short comment after it saying what the overall line is supposed to do ( //add 'FIZZ' to the output string if cnt is a multiple of 3 ).

oeb wrote: »

* For each number, we check the modulus, a zero result means that it
* is evenly divisible, so we add either 'Fizz' or 'Buzz' to the output as
* applicable. Should a number pass both tests, both Fizz and Buzz
* Will be added.

Calina wrote: »

It's not just recent grads who can't comment their code. I know people with 15 years experience who can't and who rely on the "it's self evident" defence.

But if I'm asking during a code review what the hell their code is doing and why the hell it is doing it that way, then it probably isn't self evident, despite their wails to the contrary.

There's no point in getting hung up on it being a simple coding test or whatever. How they approach that should give you a clue on how they'd approach something more complex and that really matters. I would say "it's too simple to need documentation" is not likely to be positive.

Should people be commenting like oeb and Sparks suggested?

The second, by oeb, as I've already said is inappropriate. It gives away the complete implementation. So imagine this is a method on an object. As a developer, I know in full detail what something does. There is no encapsulation here. I know the inner details of the implementation, and I can rely on this. Later on you change it. All hell breaks loose.

Obvious comments are also simply a distraction. If too many things are commented, eventually comments will be simply be ignored. Important and useful comments will be ignored. Comments will also go unmaintained, and you just made the code base larger and confusing.

What you want potential candidates to do is make believe. Pretend this is more complicated then it really is. FizzBuzz is meant to test the most basic of programming skills.

I agree with this guys sentiments:
http://weblog.raganwald.com/2007/01/dont-overthink-fizzbuzz.html

If you are using FizzBuzz and asking them to document it, well, you need to get more creative in your recruiting practices. You are also over-thinking FizzBuzz, a very silly thing to do.

What you are asking users to do, is pretend the code is more complicated, and comment as such. Believing this exercise is of much value is a delusion. Remember, you have more time than you think, FizzBuzz is best used in the pre-screening phase. If you want to test a skill, test it with something that warrants it. Get them to email it in. If they fail early, no interview is required.

Pygmalion

I don't comment nearly as much as I should, but when I do it's rarely ever explaining how the code works, or what it does, but explaining why something that isn't immediately obvious or could be overlooked was done or whether things have to be done in a certain order, and being clear about it.

e.g. Make sure you close the log file before releasing the lock. instead of Here we close the log file and release the lock

Error checking also usually gets it, not so much to explain the code or convince anyone that it's necessary, just because it's a lot easier for me to consider whether I've taken into account edge-cases and tricky errors if I can read through my error-checking in plain English.

fasty

Pygmalion wrote: »

e.g. Make sure you close the log file before releasing the lock. instead of Here we close the log file and release the lock

You could argue that you shouldn't have to document something like that in the first place with a comment.

So stuff like this, I either make sure it would be closed, or if that's not practical, try to put an assert or something to tell the programmer "you're doing it wrong"

That sort of work is worth a 100 comments.

I'm not attacking you or anything, I'm just saying when people say the code should be self documenting, sometimes compile time warnings or runtime asserts make more sense than a piece of text in a file I'll never look at.

Webmonkey

Code Complete 2 is a good book to read for such things like this.

http://www.amazon.co.uk/Code-Complete-Practical-Handbook-Construction/dp/0735619670

Comments are extremely rare in our code base here but then it is a 10 year old code base with over a million line no doubt so with comments I could imagine what it would be like.

Procasinator

fasty wrote: »

I'm not attacking you or anything, I'm just saying when people say the code should be self documenting, sometimes compile time warnings or runtime asserts make more sense than a piece of text in a file I'll never look at.

Along the same lines, unit tests can also double to be one of the best documentation you can get of how a system actually is. Properly constructed, they can tell you what the code is meant to do in usually a quite readable fashion.

fasty

And code that it unit testable (lol), would typically need to be self documenting in the sense that it would rely less on hidden state and more on dependency injection and a proper separation of concerns.

Code like that is almost much easier to use WITHOUT looking up comments because you know if you need to initialize a class, it depends on some other class and so on and so forth.

It's a shame writing them is the most tedious thing EVER. I only write them for business logic type stuff now and TDD can feck off!

An.Duine.Eile

Here is one on my favourite comments

//When I wrote this, only God and I understood what I was doing
//Now, God only knows

Pygmalion

fasty wrote: »

You could argue that you shouldn't have to document something like that in the first place with a comment.

So stuff like this, I either make sure it would be closed, or if that's not practical, try to put an assert or something to tell the programmer "you're doing it wrong"

Well you are making sure it's closed first, that's presumably what the code immediately following the comment does.
The comment just ensures that if the code is touched at some point in the future then whoever is making the changes is aware of the importance.

Both could be done, but if for whatever reason the change is made and run without debugging symbols then the assert would be ignored, and compile-time warnings only really help if it's a fairly standalone thing, otherwise it'd just be slaughtered by the output of make anyway.
Lots of text-editors/IDEs nowadays seem to try their best to make comments starting with //WARNING, //FIXME and //TODO etc. stand out as much as possible as well, so such a comment would presumably stand out more than an assert or compile-time warning.
Although I guess there's nothing to stop editors giving the same highlighting treatment to assert() and #warning, I just haven't seen it done.

fasty

Oh sorry, I misread. Hmm, I don't really see the point in that at all.

You may as well tag every line of code that opens a resource with

// Close this handle!

when really, cleanup should be managed by whatever mechanism the language of choice provides.

I've seen multiple codebases with // TODO and // FIXME from commits 5 years old and no changes to the file since then.

Regarding asserts. They're there to catch programmer error, not user errors. Your mileage may vary, but I think they're easily the best way to document usage and expected behaviour. Plus, asserts don't need debug symbols, they're just conditionally compiled into code. Which is amusingly why some bugs are created by people wrapping important code in an assert!

Regarding compile time warnings. Warnings as errors, man!

Naikon

Exceptions are an illusion of safety!

croo

when I write something I tend to write the comments first and they act as a kind of psuedo code. When I'm happy with the flow/logic I write the actual code. Any complicated formulas I tend to document too. I'm mostly coding in java these days, so other than that the only comments I write are those for javadoc and in that respect I tend to just do what others are doing to keep a consistency.