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:

So I guess the generic answer is: Those responsibilies 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 litte 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...)
EditText of this page (last edited April 6, 2004)
FindPage by browsing or searching

This page mirrored in WikiPagesAboutRefactoring as of July 17, 2004