Spotting code smells (was: Re: Small Methods - a ramble)

At 03:50 PM 1/14/02, David Alan Black wrote:
> > I read Fish because, well, I'm weird. But I'm also interested in the ways
> > that code unintentionally tricks the reader. The rather simple techniques
> > of affective stylistics might help programmers write better, be better at
> > finding "code smells".
>
>"Unintentionally tricks" -- now you're *really* ascribing agency to
>the text :-)  But still... can you elaborate on what you're suggesting
>here?  I'm not sure, in practical (coding, stylistic, work habit)
>terms, exactly what you mean.

I am mainly talking about work habits.

Let me use an example. I need to work up a better one, one of improving my 
own code. But this will have to do for now.

About a year ago, I was trying to figure out why a beta version of the 
VisualWorks Smalltalk distributed object system didn't work on my machine. 
I set a breakpoint, and quickly stepped my way to this code (rewritten into 
Ruby):

class NamingService
   ...
   def bindingSet
      bindings.to_a
   end
end

I already knew that bindings was an instance variable, a Hash. So I "knew" 
that the method 'bindings' was just an accessor, maybe with lazy 
initialization. I didn't step into it. I stepped over it. It took me a 
ridiculously long time before I got a clue and looked at the definition of 
NamingServiceRoot#bindings. It was something like 27 lines of code, and it 
was where the problem lay.

It was a sort of lazy initializer, I guess. Except that it was initializing 
much more than the value of the instance variable. It was actually going 
out and setting up network connections, forking off processes, etc. My 
problem was due to a timeout value too low for my NT box.

I claim it was bad code. But I'm not claiming it was produced by a bad 
coder, or even that it was all that surprising. I asked Ralph Johnson if he 
could see himself writing that kind of code. (Ralph is a notable member of 
the Smalltalk community, and quite familiar with the customs of that 
community like small methods and intention-revealing names.) He said he 
could. Maybe that was the most convenient place to put the startup code 
early in development, and there was never an obvious better place to put 
it. So an ugliness could get started, grow bit by bit. (Note that this 
conversation was while we were walking to lunch, so he's not looked at the 
actual code, and I may have misunderstood an off-the-cuff judgment.)

So what could make an ordinary mortal decide at some point to make the code 
better?

A couple of triggers are obvious. The code is big. Splitting it up might 
leave the accessing part of the method behind and put the 
subsystem-initialization code somewhere more obvious, like a method called 
by "new". Or the coder might be reminded of Beck's wonderful _Smalltalk 
Best Practices Patterns_ and its rule about picking intention-revealing 
method names. That could lead to taking part of the code and putting it in 
a better-named method.

A third thing might be a question derived from affective stylistics. 
Specifically, some Fish-ian questions to ask of a method after you've 
finished it would be:

"Suppose someone reading this program hits a call to 'binding'...
       ...
       ... foo.binding ...
       ...
What do they know at that point? What assumptions are they making? Having 
read the call to 'binding', how might they be misled? How could they be 
surprised later as they read more code?"

*Maybe* thinking along those lines would make the coder realize that the 
reader will probably be surprised that network connections are being set up 
by an accessor method.

In a way, I'm thinking of building on Beck's foundation. His book is 
eloquently convincing that the purpose of code is to communicate to other 
programmers. He gives solid widely-applicable rules for writing better 
code. Follow those rules, and your code's readers will be served. But the 
rules are not (can not be) in terms of specific readers or specific code. 
The question is whether this literary criticism mumbo-jumbo allows you to 
discover additional things you'd otherwise overlook.

Of course, all this is worthwhile only if it helps enough often enough, 
doesn't take much time, and can be done as just another part of normal work 
habits.

It can fit in well, I think, with my work habits. I always have a cleanup 
phase. After I think the code is ready to inflict on others, I try to 
reread it with a fresh eye and tidy up. I especially pick better names for 
variables, methods, and classes. I can't seem to pick the right ones while 
coding: I just see things differently in programming mode than in 
thinking-about-the-reader mode. And I'm sure I couldn't ask Fish-ian 
questions while programming.

(Should this go off-line?)

--
Brian Marick, [e-mail]
www.testing.com - Software testing services and resources
www.testingcraft.com - Where software testers exchange techniques
www.visibleworkings.com - Adequate understanding of system internals