[Grace-core] Code Smells

[Kim Bruce]

Andrew,

Actually, I mainly agree with you.

In our original objectdraw library, we used Location extensively throughout the library, including the places you note.  Here is a listing of methods from the "WindowController" class:

 void begin() 
 void onMouseClick(Location point) 
 void onMouseDrag(Location point) 
 void onMouseEnter(Location point) 
 void onMouseExit(Location point) 
 void onMouseMove(Location point) 
 void onMousePress(Location point) 
 void onMouseRelease(Location point) 

There are not versions of those methods using x and y parameters, so students must use Locations from the beginning.  (Their first programs are overriding those methods.)

In starting to write these libraries here I decided to use x and y because I don't yet have confidence in the implementation, and didn't want to add more possible points of failure.  I have spent more hours than I'd like to think trying to track down bugs that have popped up in my code (many of which are actually implementation issues), so keeping with primitives where possible seemed safer and left me with fewer things to worry about.  

Only this morning did I figure out how to reliably do inheritance across module boundaries, and that was after many hours of struggling with obscure errors!  Also, at this moment type definitions cannot be exported across boundaries, so all of my code starts with repeated type definitions.  Using primitives meant a bit less stuff to copy across modules.

So, yes, I intend to eventually get rid of all those x's and y's and replace them by Locations.  (I also had defined a type Boundary that I gave up using when it became too painful to copy stuff across boundaries.  That will likely come back again as well.)

As for the names of the methods above, you are correct that they are named from the point of view of the implementor of the class rather than the caller.  (By the way, the corresponding names in standard Java are mouseClicked, mouseDragged, etc.).  

That choice was actually deliberate here for pedagogical reasons.  These methods are never invoked directly by the programmer.  Instead they are generated by the system in response to user actions.  So, while other methods are typically "used" by programmers, these are never "used" by them, they are defined by them.

Thus, while I agree that generally the naming should be from the point of view of the user, there is a good argument to be made that in these special cases, it makes pedagogical sense to name them from the point of the implementor.

I do invite you to take a look at the Javadoc for the Java-based object-draw library at:
	http://eventfuljava.cs.williams.edu/library/objectdrawJavadocV1.1.2/index.html
Let me know if there are design aspects of the library that you don't like (I know some things have been controversial in the past) and we can discuss them.

Talk to you Thursday at 1:30 p.m.?

Kim



On Jun 5, 2012, at 2:22 PM, Andrew P. Black wrote:

> 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
> 
> 
> 
> 
> _______________________________________________
> Grace-core mailing list
> Grace-core at cecs.pdx.edu
> https://mailhost.cecs.pdx.edu/mailman/listinfo/grace-core