Commenting Interview Question

[refactored from InterviewingStrategy, 9/8/01, SteveHowell, good candidate for deletion

The page was a fun exercise, but it doesn't really have any focus. I will leave it for now, though. Is there something in particular on this page that you would like to keep for posterity? Perhaps we can refactor it a bit. -- SteveHowell]

Should every function in a C module have a block-level comment?

No. What would a block-level comment add to

 extern int clockTicksSinceReset(void) {
   return clockCurrentTick() - clockLastResetTime();
 }

I would ask:

what unit are ticks?
what do you do when the clock gets reset and your ticks are no longer valid?

Good questions, which indicate that I chose a bad example.

I intended that clockLastResetTime returns a number of ticks; but I couldn't, off the top of my head, think of a good name for it that indicated this and wasn't intolerably verbose.
I intended that the clock doesn't get reset.

As I say, it was a bad example. I felt bad about it as I was writing it but was too lazy to improve it. Sorry.

''No, it's a great example, because it is so seemingly simple and the names seem so good it seems like it doesn't need documentation. Which is why i jump so hard on the no documentation camp. Documentation is largely about answering questions developers will have about any section of code. Name selection cannot possibly answer these questions. The tick unit is crucial. It's an obvious first question. The second question is a problem that happens in many systems. With no documentation, it shows you didn't even consider the case and the system can fail in very subtle yet brutal manner. Assuming that it won't happen is a horrible design decision, but it's a design decision that must be documented. Even the smallest line of code exists in a context that must be explained. Naming most of the time can't explain the decision process and all the other important things about the code. The code is almost the least important part.''

Okay, so here's the challenge. Write a good comment for that code. -- SteveHowell

The perpetrator writes:

Actually, what it shows is what I said it shows :-). You're assuming all sorts of things about the design of this hypothetical system that don't correspond to what I had in mind when I was thinking about the example. For instance: by "clock" I didn't mean something like "clock of this simulated device", but something more like "monotonic increasing time reference throughout a given simulation". Again, "clock" was a lousy name; that's one of many reasons why this is a bad example. If you take a snippet of code out of its context then it can never be self-documenting (even the "ellipse" example isn't - what units are the coordinates and radius in? where's the origin? does y increase upwards or downwards? what are we drawing on? and so on, and on).

By the way, I'm decidedly not in the "no documentation" camp. But the right place to specify, for instance, what coordinates are used for that drawEllipse method is almost certainly not in a method comment for it. The same coordinate system will be used by the drawRectangle, drawLine, drawPolygon, drawPoint, and drawWombat methods. Explaining the coordinate system once per method is a terrible violation of OnceAndOnlyOnce (or, if you prefer, DontRepeatYourself), and that's bad regardless of your preferences for comments generally.

I'm being over-defensive now, so I'll stop. Just one remark. "it shows you didn't even consider the case": no, it doesn't, because I did consider the case and thought "no, it's only a throw-away example, it doesn't matter". The real lesson from this is that extrapolating from someone's throw-away stuff is dangerous. In an interview, of course, this would all come out in a more helpful way.

Oh, and in answer to Steve's challenge: the fundamental problem is that the methods are misnamed and there's a lot of context missing, but I propose:

 /* Number of master simulation clock ticks since the last
  * reset of the simulated device.
  */
 extern int clockTicksSinceReset(void) {
   return clockCurrentTick() - clockLastResetTime();
 }

The meaning of "tick" and the peculiar definition (i.e., broken name) of clockLastResetTime would be documented elsewhere in the file.

Sorry, I was trying to challenge the second guy, not the first. I wish you guys would sign your posts. The other guy said, look, this piece of code shows how important comments are, but he doesn't actually offer a good comment for the code. You do. -- SteveHowell

You can't in one breath say it's just throw away code and ask how it could be commented. From your challenge and the attention to naming there is every reason to believe this to you was good code. When I look at code, I have questions. Where are those questions answered? Explaining what happens if the time changes in the reset makes sense. Why would it be any place else? The ticks I almost buy your argument. But without using a typedef or have @return documentation there is no reason to expect it to be magically documented somewhere else. -- AnonymousDonor

Lesson Learned From Silly Debate Above

I think the preceding debate over hypothetical code illustrates the importance of a PairProgrammingTestDrive during software interviews. I'm guessing every participant in the above discussions writes reasonable comments, because all posters seem intelligent to me and capable of good judgment. But, there's no way to truly gauge it from debating a contrived clock-tick code snippet totally out of context. We would all learn a lot more about how we program by sitting down together in front of the computer and writing actual code. -- SteveHowell

The same coordinate system will be used by the drawRectangle, drawLine, drawPolygon, drawPoint, and drawWombat methods. Explaining the coordinate system once per method is a terrible violation of OnceAndOnlyOnce (or, if you prefer, DontRepeatYourself), and that's bad regardless of your preferences for comments generally.

Although I agree with the OnceAndOnlyOnce principle, I have trouble agreeing this statement. When you are browsing through the API reference (e.g., in Java, generated by javadoc from the function comments), it is nice to see "x, y measured from top-left corner" in all the drawXXXX methods.

For real life examples, when I work with any class with get(int index) or set(int index) methods, I will like to be reminded whether the index is 1-based or 0-based. I can probably find it in the class comment, but when I am hunting through the reference, having it documented in the function itself is very useful.

-- OliverChung