Re: Reek's critique
Kevin Rutherford
Thanks for the question! I've copied the (sparsely populated) Reek
list in case anyone else has similar thoughts...
On Wed, Jan 13, 2010 at 12:58 PM, Hugh Sasse <h...@dmu.ac.uk> wrote:
> I've found reek to be useful in reshaping my code, but I'm a bit
> puzzled by its flagging up "utility functions". I usually end up
> creating these in response to the long method criticism, or the
> duplicated code criticism. I wonder if it would be worth adding
> rdoc docs to explain briefly what the problems are with these
> smells? I can understand that a utility function should only be
> private, although, if one class needs it and it doesn't change the
> state of anything in a class, or globally, then why not make it
> public so other classes don't need duplicate the code? Otherwise,
> what is the problem with this smell? I've read the comments in
> Utility_function.rb, but they tell me what it will detect, not why
> this is classed as a smell. Maybe the utility functions are better
> placed in Modules, because modules which don't use state (instance
> variables) can be added with impunity anywhere?
>
> If it is all about reducing duplication and picking the right names
> for things, then a correctly named utility function could add
> clarity to a code base. Inches shouldn't need to know about
> centimetres, or vice versa, but a conversion function which would
> not change state, is still a useful thing to wrap the 2.54 constant
> and keep it in one place. Then if the BSI reform the imperial
> system to round values to the nearest millimetre, then there is only
> one place to make the change.
>
> Thank you,
> Hugh
Here's the theoretical answer:
A Utility Function is one that makes no reference to the instance
state of the object on which it lives. Which means that it is at best
only poorly cohesive with that object. So there is likely to be
another object, referenced within that method, which would be a better
home for the method.
To that end, Reek only calls out a UtilityFunction for methods that
make more than one call elsewhere.
In practice, in my own work, I have disabled Reek's UtilityFunction
check. I reason that the method should live close to (ie. in the same
object as) its caller until another class needs it too (or until
another class creates similar or duplicated code). At that point the
method is free to move to somewhere useful to /both/ client objects.
So I'm happier with low cohesion than I am with Duplication; and I use
the same arguments about FeatureEnvy too: I leave code close to where
it is called until I need to share it among multiple classes.
Cheers,
Kevin
--
http://www.kevinrutherford.co.uk -- agile, TDD, XP, lean, TOC
Ashley Moran
On Jan 13, 2010, at 8:46 pm, Kevin Rutherford wrote:
> In practice, in my own work, I have disabled Reek's UtilityFunction
> check. I reason that the method should live close to (ie. in the same
> object as) its caller until another class needs it too (or until
> another class creates similar or duplicated code). At that point the
> method is free to move to somewhere useful to /both/ client objects.
> So I'm happier with low cohesion than I am with Duplication; and I use
> the same arguments about FeatureEnvy too: I leave code close to where
> it is called until I need to share it among multiple classes.
What I don't know, is how can you be sure that you in the future (or another developer in the present) will be able to find the duplication when it does occur? Allowing low cohesion now seems to increase the risk of duplication in the future. Is this a real risk?
Ashley
--
http://www.patchspace.co.uk/
http://www.linkedin.com/in/ashleymoran
Kevin Rutherford
>> To that end, Reek only calls out a UtilityFunction for methods that
>> make more than one call elsewhere.
>
> I've found this is often calls to methods of the arguments, if I'm
> remembering this right. The rules about belonging should be
> different in that case, shouldn't they? Fiddling with the arguments
> doesn't break the law of Demeter, presumably because it is not a Bad
> Thing. So why would it be a bad thing here? Maybe this is an
> inference too far on my part...
You're right, it will often be calls to methods on the arguments. The
idea is that a method call such as fred(a, b, c) would be more
cohesive as a.something(b, c) -- reducing the length of the parameter
list and moving the code onto the most-referenced object.
>> In practice, in my own work, I have disabled Reek's UtilityFunction
>> check. I reason that the method should live close to (ie. in the same
>> object as) its caller until another class needs it too (or until
>> another class creates similar or duplicated code). At that point the
>> method is free to move to somewhere useful to /both/ client objects.
>
> I'm thinking that would be a module or adapter pattern, as a suitable
> home. But proliferating patterns is a smell of a different colour.
Well, if it's a Utility Function I would tend to move the method onto
one of its arguments.
Kevin Rutherford
> What I don't know, is how can you be sure that you in the future (or another developer in the present) will be
> able to find the duplication when it does occur? Allowing low cohesion now seems to increase the risk of
> duplication in the future. Is this a real risk?
Yes, it's a big risk, and is the primary motivation given for fixing
Utility Function and Feature Envy "early". So I run Flay and Simian on
all of my code after every commit. Not foolproof, but not bad. I also
look out for Shotgun Surgery, treating it as a sign of a duplicated
responsibility and fixing it as soon as I see it.
Matteo Vaccari published a kata style this week that might prevent
Shotgun Surgery: http://matteo.vaccari.name/blog/archives/293
Seems like it might be worth a try...
Cheers,
Kevin
Ashley Moran
On Jan 14, 2010, at 8:34 pm, Kevin Rutherford wrote:
> Matteo Vaccari published a kata style this week that might prevent
> Shotgun Surgery: http://matteo.vaccari.name/blog/archives/293
> Seems like it might be worth a try...
I like the look of that!