I’ve been refactoring a lot during the festive break, and I’ve noticed that in many cases it was more difficult than I would have liked. Today I think I figured out the reason for that: I use the TemplateMethod pattern too much.
When I see a duplicated algorithm, it seems that my natural tendency is to push up the skeleton into a superclass. This creates an inheritance relationship within the algorithm, which in turn makes it harder to change. Later, when I do need to change the algorithm, I have to change the superclass and all of the subclasses at the same time. For example, one particular superclass in reek contained three or four template methods, which made the subclasses look quite odd; and each little complex of template-plus-overrides significantly hampered design change in each of the others.
Looking back over my programming career I see that I’ve always had this tendency — I can see it in my old C++, Java and Ruby code. I wonder why? Is it the cost of extra classes, or my mathematical background, or coding habits ingrained before the rise of object-oriented languages? Who knows (and who cares).
So, note to self: Break out State/Strategy objects (and in Ruby this includes Procs) instead of always relying on TemplateMethod.
I spent an hour or so this afternoon refactoring some code deep inside reek. I wanted to simplify the code that checks for the Uncommunicative Name smell, but it just wouldn’t fall out as clean as I would like. And at some points it seemed that I was going around in circles — a name is represented as a Symbol here, needs to be a String there, but when I add a call to
to_s something else seems to need it back as a Symbol again. It’s the old feeling of squeezing water in a balloon: squeeze in any one place and the problem seems to pop up somewhere else. I put the problem down to tiredness and took a break — actually I took a drive in falling snow, which is not necessarily a cure for tiredness!
But the change of scene cleared my thoughts, and I realised what was going on: The troublesome names were a case of the Primitive Obsession smell. Several classes throughout reek “know” when a name travels around as a Symbol and when it travels around as a String. I needed a new class, Name, to hide the details of the actual representation. I added it (following the recipe in the ruby refactoring workbook, you’ll be glad to hear), and all was well. The new class even acquired some behaviour along the way, so all is well.
So, nothing new there; why am I even writing this at all? Because I asked myself why it had happened — why had such a huge case of Primitive Obsession escaped my attention for so many weeks, and why didn’t I pick it up this afternoon when it hit me repeatedly between the eyes (can a smell do that)? Is this another manifestation of my anosmia? Am I smell-blind to Primitive Obsession? I don’t think so.
Every day in my coaching practice I see code riddled with this same smell. I point it out, we have a quick ad-hoc training session and a couple of lunchtime dojos. I can spot this problem easily. The difference is that the teams I work with all use C# or Java or C++, where the types are stated clearly in the method signatures. But in Ruby that’s not the case: when you read a method in Ruby code, you have to infer the types of the actual arguments by looking at how they are used. Parameter names don’t always help here either (in the reek source code above, sometimes the names were called ‘sym’, other times ‘name’).
So I contend that dynamic typing makes Primitive Obsession harder to diagnose. Do you find that too? Does this smell linger longer in Ruby code than in Java code, for example?
And if my contention is true, what new diagnostic tools do we need? If we don’t have statically typed method signatures slapping us with the wet fish of
string, are there any simple alternative cues we can look for?
Pat Eyler’s On-Ruby blog today features an interview with me about reek — why I wrote it, and how I use it and related tools in my development practice. It was really nice to be asked, and Pat has done a great job — thanks Pat!
(Next week, Pat will post an interview with Bill Wake and me about the Ruby Refactoring Workbook…)
I’ve just release reek version 0.3.1 with the following wonderful smelly goodness:
- Uncommunicative Name now checks instance variables more thoroughly, and also warns about names of the form ‘x2’.
- There’s a new warning about multiple identical calls within a single method…
- …and consequently the scope of Feature Envy warnings has been reduced, so that it now only covers overuse of lvars within a single method. Hopefully the warnings will be a little less rabid than in previous versions!
- Each checked code smell now has rdoc comments explaining what each smell is about and providing a simple example.
- Chained iterators are no longer mis-reported as nested.
Available now at a gem repository near you.
reek doesn’t offer a direct way to specify that your source file is UTF-8 or such, but there is a workaround if you need it. reek 0.3.0 introduced
Reek::RakeTask, which makes it easy to run reek from a rakefile. And this task has an attribute
ruby_opts, which is an array of strings to be passed as arguments directly to the Ruby interpreter. So you can use reek to check for smells in non-ASCII files by adding something like this to your rakefile:
Reek::RakeTask.new('utf8_file') do |t|
t.source_files = 'my_utf8_file.rb'
t.ruby_opts = ['-Ku']
Not ideal, I know; so if there’s sufficient outcry, I’ll consider adding either a -K or a –ruby-opts to reek.
I’ve just released reek version 0.3.0. Highlights in this version include:
- The main news is the arrival of a new smell: I’ve included a first naive check for the Control Couple smell.
- I’ve also revised the command-line: reek now only checks sources passed on the command line — it won’t go searching for files in the current directory and subdirectories any longer.
- Code snippets can be supplied on the commandline; if a command-line argument isn’t a filename, reek will assume it’s Ruby source code, and will look for smells in it.
- If multiple files generate warnings, reek now lists them with headings, so you can more easily see which files had problems. Each heading also lists the number of warnings raised against the file.
- There’s now a very naive Reek::RakeTask to run reek from rakefiles.
- And finally, reek now exits with status 2 when smells are reported; this should help you integrate a call to reek in your continuous integration builds.
To get your copy:
gem install reek, or
gem update reek if you’re a reek user already. To use it, try
or similar. Or add the new Reek::RakeTask to your rakefile. As always, all kinds of feedback is very welcome.
Some of you noticed that reek wasn’t working on Windows. That’s because reek depends on ParseTree, which depends on RubyInline, which was broken on Windows. But now RubyInline is fixed (version 3.8.1), so a quick
gem update fixes the problem.
Feature Envy is the smell (anti-pattern) in which one code fragment makes more use of another object than it does of itself. reek currently (version 0.2.3) includes a very naive check for Feature Envy: For each method inspected, reek counts the messages sent to each recipient and also counts the references to self; if self has the highest count, reek doesn’t report feature envy.
Here’s a contrived example:
When it sees this code, reek says:
[Feature Envy] Parcel#addressee uses @person more than self
That’s because the method sends two messages to
@person, so this code fragment should be moved onto that object:
Now fewer messages are sent overall, so the Feature Envy is gone.
The current algorithm is quite naive, and reek will certainly report false negatives sometimes. I hope to be able to refine the algorithms and gradually make reek smarter; for example, I’ve considered adding a configuration option so you can turn off certain smells for certain methods, but somehow that feels like the start of a slippery slope. This winter I hope to find the time to work on the algorithms a bit more, so if you do find reek reporting a false negative (for any smell), please send me a code sample — or preferably a new test.