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:

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:

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
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.

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

Sudhindra Rao said…
Hi Jake,
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.
Piers Cawley said…
You're overcomplicating that inject, and it doesn't short circuit like the long version. Try something like:

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
Yi Wen said…
This comment has been removed by the author.
Yi Wen said…
Hey Jake. Nice post. But why not just def your method as x == y, or if this is not the case, then do x.some_conditions_equal? y where some_condition should indicate why you want to write this method. With good unit test/rspec, people will know what this method is doing immediately.
Toby Tripp said…
I think people may be missing the point here.

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? ;)
Jake Scruggs said…
Toby's right, I just wanted to talk about about using common sense when looking at metrics. The actual method was simplified and changed so as not to be posting client code on a public blog. I'd also like to apologize for not giving Toby a shout out for discovering this method in our code and letting me blog about it. I need to make sure I keep sitting near him in the team room so I can get more ideas for free.
Anonymous said…
%w(foo bar baz).all? { |meth| x.send(meth) == y.send(meth) }

Popular posts from this blog

Point Inside a Polygon in Ruby

SICP Wasn’t Written for You

What's a Good Flog Score?