The Method Hit List
In a previous post I talked about how my team has a daily metrics build that reports on our code quality. One of the things we measure is the Flog score of our methods (Flog is a ruby program that evaluates the complexity of your methods). We have a standing developer task to look at the top five worst methods in our application and reduce their complexity. Every iteration we try to spend some time looking at a few developer tasks, which are things that don't provide any direct business value but do make our code better, automate some manual process, or fix some technical problem not tied to a bug.
Yesterday Chirdeep and I refactored the worst method in our app and while it wasn't easy it produced a number of benefits:
First, about halfway though the refactoring we came upon some twisted logic that seemed as if it could never evaluate to false. After pulling in Ali for some consultation, we realized that indeed it would always be true and therefore was a bug. We fixed it and moved on.
Second, we noticed that the object inherited from another object, but overwrote the constructor with a strange one that took different arguments. This had the effect of making other objects interact weirdly with this class in order to deal with its 'sorta' inheritance. We managed to get rid of the custom constructor and have it rely on it's super class.
Finally, we noticed a nice clear division of labor in the complex method so we were able to extract a method that had a single responsibility. So we ended up with two methods that shared the complexity of the original one.
I would have been happy if all we accomplished was the last item. However, when you take a look at your most complex methods you often find bad stuff hiding amongst the craziness.
It's also important to note what we didn't do:
We did not simply extract out every 5 lines into a method. While that would have improved our Flog score per method, it would been the code equivalent of sweeping dirt under the rug. Methods should, as much as possible, have a single responsibility. If they don't then all you're doing is moving a bunch of complex stuff into many different arbitrary places, instead of just one place. Which is actually worse. So, like all metrics, complexity is something to pay attention to but you don't want to slavishly chase after good numbers.
Yesterday Chirdeep and I refactored the worst method in our app and while it wasn't easy it produced a number of benefits:
First, about halfway though the refactoring we came upon some twisted logic that seemed as if it could never evaluate to false. After pulling in Ali for some consultation, we realized that indeed it would always be true and therefore was a bug. We fixed it and moved on.
Second, we noticed that the object inherited from another object, but overwrote the constructor with a strange one that took different arguments. This had the effect of making other objects interact weirdly with this class in order to deal with its 'sorta' inheritance. We managed to get rid of the custom constructor and have it rely on it's super class.
Finally, we noticed a nice clear division of labor in the complex method so we were able to extract a method that had a single responsibility. So we ended up with two methods that shared the complexity of the original one.
I would have been happy if all we accomplished was the last item. However, when you take a look at your most complex methods you often find bad stuff hiding amongst the craziness.
It's also important to note what we didn't do:
We did not simply extract out every 5 lines into a method. While that would have improved our Flog score per method, it would been the code equivalent of sweeping dirt under the rug. Methods should, as much as possible, have a single responsibility. If they don't then all you're doing is moving a bunch of complex stuff into many different arbitrary places, instead of just one place. Which is actually worse. So, like all metrics, complexity is something to pay attention to but you don't want to slavishly chase after good numbers.
Comments