When You Should Ignore Metrics
Our team has been doing a bang up job of reducing our complexity through our hit list (All of our methods are ranked by Flog score and we spend some part of every iteration picking the worst methods and trying to refactor them.). But sometimes we run into a situation where we prefer a high Flog score to a low one. For instance, this bit 'o code:
which had a Flog score in the 60s. We could have changed it to this:
After some discussion with the team we decided that while Flog is very good at telling you which methods to inspect for badness, it is not always true that a better score makes a for better method. The same can be said for all metrics, really.
def is_something?(x, y)
x.foo == y.foo &&
x.bar == y.bar &&
x.blah == y.blah &&
# and 10 more lines of the same
end
which had a Flog score in the 60s. We could have changed it to this:
Which gave a us a much lower score. However that first method is crazy simple. Merely glance at it and you know what it's doing. The 'less complex' method? Even an experienced ruby dev would need a moment or two. As for a new dev... I can remember a time not so long ago when I found an inject in some source code and was pretty confused even after I looked up the Rdoc. It's one of those methods that's so powerful and flexible that the documentation only seems to make sense after you understand it.
def is_something?(x, y)
["foo", "bar", "blah", # and so on ].inject(true) do | a, b |
a && x.send(b) == y.send(b)
end
end
After some discussion with the team we decided that while Flog is very good at telling you which methods to inspect for badness, it is not always true that a better score makes a for better method. The same can be said for all metrics, really.
Comments
Trying to talk about something that is abstract enough to apply in every situation is hard. As is what you are trying to say here with this example. I think if your Flog score sucks then there is more than just implementation of that one method that we need to look at.
Considering a more concrete example for this discussion
if we say your method was
def is_same_employee?(x, y)
x.name == y.name &&
x.age == y.age &&
x.employer == y.employer
// and ten more such comparisons
end
I think the problem is deeper than we see. Probably you have a bloated object model.. meaning the employee has to perform so many checks to make sure it is the right one.
Identifying smaller objects or identifying methods with a better purpose could help.
like so
def is_same_age?(x, y)
x.age == y.age
end
def is_same_name?(x,y)
x.name == y.name
end
etc.
That way your code still remains readable and could help in getting a lower Flog score.
def is_something?(x,y)
%w(foo bar blah ...).each do |each|
return false unless x.send(each) == y.send(each)
end
return true
end
Which is a pretty straightforward loop over the properties.
Incidentally, why write it as a static method like that?
class Object
def is_something?(other)
matches_attrs(other, :foo, :bar, ...)
end
def matches_attrs(other, *methods)
methods.each do |each|
return false unless send(:each) == other.send(each)
end
return true
end
end
The method in question happened to be the one with the highest flog score that day. Flog complained that the method was calling the '==' method too many times (ten is an exaggeration, it was closer to five). Changing the implementation from boolean intersection to iteration lowered the flog score but significantly reduced readability.
The app has a reasonably good flog score for a code-base of its size. The method in question is a simple comparator. There's no hidden object-rot to be found here.
I don't want to put words in Jake's mouth, but I think the point is that chasing metrics suffers from diminishing returns.
---
Nice post Jake, but can a guy get some credit? ;)