Comment Example

Here's a Smalltalk method with a comment, followed by a bit of rewriting using reasonable coding standards. Help me with suggestions for useful comments.

Thanks!

expRpt
"Print an expense report consisting of the personal information
of the submitter, the travel details with connection to the trip 
request, the detail item summary, and a total."

self
persInfo;
travInfo.
detailItems do: 
[ :each | self dtlInfo: (detailItems at: each)]

OK, I'd agree that this needs the comment. What if it looked like this:

printExpenseReport
self
printPersonalInfo;
printTravelInfo;
printDetailItems;
printTotal

I wouldn't find the comment to be of much value any more ... what would be a useful one?

What if some of the inner methods looked like this:

printPersonalInfo
self
printName;
printAddress;
printDepartment

printDetailItems
detailItems do: [ :each | self printDetail: each]

printDetail: aDetailItem
self
printDescription: aDetailItem description;
printAmount: aDetailItem amount.

And so on ... what kind of commenting would be needed? -- RonJeffries

Can I change the formatting at all? What the hell is the format? Do I get subtotals or just grand totals? Can I attach a filter to select just a subset? Where will it print to? How long will it take? How do I cancel it? Any special printer setup like labels/checks? What size paper is assumed? Can I email the report and print it out?

Yes, questions like these tend to pop up in my mind as well when I see code like this. It seems too simple to be feasible in the RealWorld, where you need to support demands like those. So I thought I'd attempt to answer these questions myself, hoping for comment from XP people.

Obviously there is no sign of anything like this in the code as shown. But it's easy to see how it could be modified to support this, if required:

• Can I change the formatting at all? What the hell is the format?
• If there is need for different formats, provide a formatter superclass/interface which provide generic methods for formatting reports. Implement to provide different formats. Pass to the report printer, either at method invocation or object creation.
• Do I get subtotals or just grand totals?
• Which do you want?
• Can I attach a filter to select just a subset?
• Would you like to be able to?
• Where will it print to? How long will it take? How do I cancel it?
• Collect the result of the printing, for example in your formatter. Produce a printable document from the result, which is put on your printer's print queue. The print queue gives you the control and information you ask for.
• Any special printer setup like labels/checks? What size paper is assumed?
• Handled by your printable document, your print queue, and/or your printer object.
• Can I email the report and print it out?
• Supply an e-mail formatter when printing. Submit the result to your SMTP service.

So I guess the generic answer is: Those responsibilities would be distributed to other classes as appropriate, with minor modifications to the current class. Very interesting. So perhaps code like this IS feasible. Comments? -- DanielAborg?

---

The original comment essentially duplicates what the method does (or is supposed to do: where's the total?) As such it doesn't add much to the code itself, once the cryptic naming is addressed. (Given a commenting regime, such comments can still be useful in scanning the code by reading the more readable [to some] English version, but that's a smaller matter).

A more interesting comment would describe the context of the method rather than just what it does literally. For instance, the specific report form (just "Form 7256A", or perhaps,if it's the only place in this system this reference occurs, a more detailed specification of that form, or a pointer to it). Some description of how the current object gathers the requisite information might be in order (it appears to have it all right at hand, but for instance the total calculation might be a side effect of processing the detail items, which would be good to know).

Of course, nobody's saying that every method needs a comment; this one is short, and, especially as modified, reasonably to the point. Much of the information above, even if appropriate, might well be hung on the class rather than a specific method; as long as it's somewhere. -- JimPerry

---

A category of comment which might conceivably be judged valuable is the class comment, or more generally the SummarizingComment?, as in the example below from an experimental VM written in Java. Even though... please read on.

// aastore - pop value, index and array ref, store value at index

public class aastore extends OpCode {

public void execute(StackFrame sf) {

Object value = sf.pop();
int valueIndex = sf.popInt();
Object[] valueArray = (Object[])sf.pop();

try {
valueArray[valueIndex] = value;
}
catch (NullPointerException npe) {
sf.propagate(npe);
}
catch (ArrayIndexOutOfBoundsException aioobe) {
sf.propagate(aioobe);
}
}

}

The method itself has a lot of necessary, but distracting, syntactic noise (mostly to deal with exceptions); summarizing the "story" in a comment at the top helped me remember what was supposed to be going on without having to read the code, which I tended to find painful.

I originally wrote these remarks about 6 months ago, prior to any experience with test-driven design and the philosophy that TheSourceCodeIsTheDesign. Looking back, I am inclined to think that the things which looked to me as indicative of the need for comments are more properly understood as poor design.

Today, I would be more cautious about relying on Java's own exception mechanism to generate the exceptions that my own VM has to throw; that's too clever by half. I would have null and bounds checking in separate small methods. I would probably think twice about generating as many flyweight classes with little responsibility of their own as the Java spec defines opcodes; too clever by half, again.

-- LaurentBossavit

Why is the class called aastore instead of PopValueIndexArrayStoreValue? -- HelmutLeitner

Probably because aastore is an opcode in Java and he's using dynamic class loading/resolution ? --RodneyRyan?

Yup. Another thing I might do differently. There was an initializer method somewhere that went something like...

public abstract class OpCode {

 static {

    table = new OpCode?[256];

    //  op         len kind  pop push    name               var flags

    op(ACONST_NULL, 1, CONST, 0, "a",   "aconst_null",      0, 0);
    op(ICONST_M1,   1, CONST, 0, "i",   "iconst_m1",       -1, 0);
    op(ICONST_0,    1, CONST, 0, "i",   "iconst_0",         0, 0);

(...etc...)