[Grace-core] Code Smells

[Andrew P. Black]

Dear Kim,

I'm going to rush in where angels fear to tread, and suggest that your code samples stink!

Well, not really stink.  But they do give off a whiff of two code smells that I try to make my students aware of, and which I would rather not detect in our libraries.

The first smell is what Fowler calls "primitive obsession".  That's not quiet the right name in Grace, because we don't have Primitive values, only objects, but it's the same idea.

Your library code defines several methods like

	method onMouseClick(x : Number, y : Number)

Why does this method have two parameters that are numbers?  Conceptually, shouldn't the parameter really be a Point object?  (Well, it might be a mouse Event object, containing perhaps a point and a timestamp and a set of modifier keys but perhaps you don't yet have mouse events in your framework.)   Similarly, I would expect that there would be a way of creating a Rectangle from two points, one representing the size of the rectangle, and the other its location.

One practical reason for using points is that they have behavior.  So instead of wrotuing, for example,

	print "mouse was clicked at ({x},{y}) in DrawingCanvas"  

one can write 

	print "mouse was clicked at {p} in DrawingCanvas"

because points will have an asString method that answers the (x, y) formatted string.  If we do this uniformly, there is exactly one place in the program that decided on the print format for points.  (Should it be (x, y) or x at y ?  I don't know, but when we change our minds, that change should be captured in exactly one place.)

The second smell is one that Fowler doesn't name, but I'm going to call it "egotism".  It captures the idea that a method has been named from the point of view of the receiver, rather than the point of view of the client.   Suppose that I have a canvas in hand, and I want to pass it a mouse click at point p.  It seems to me that it's natural to say

	canvas.mouseClickAt(p)

but not

	canvas.onMouseClick(p)

The idea that _ON_ the receipt of a mouse click _I_ have to do something is the internal viewpoint of the canvas, not the external viewpoint of the clients.  The clients just tell the canvas that a mouseClick has occurred.  The "At" suffix suggests that the argument is a location.   Naming  methods is subtle; it's discussed at some length in  "Smalltalk with Style" and "Smalltalk Best Practice Patterns", but perhaps the importance is best captured by the existence of Ward Cunningham's "Program with a Thesaurus" pattern.

This matters to me because poor names are one of the things that makes Java code much harder to read than Smalltalk code.  I think it's because most of the early Java libraries were written by re-tread C programmers, who had a very imperative point of view.   Unfortunately, they set the standard that those that came later followed.  Let's do better with Grace!

	Andrew