Remembering Refactoring
In a prior post, we extracted the details of how to tune a guitar out of the Guitar class. We moved it to a separate Tuner class. In that telling, we did so because it spoke to our sensibilities. It afforded us more space in the Guitar class to focus on other responsibilities. It gave a central location to focus on tuning.
What if those justifications weren't enough? What if we needed metrics to give an explanation for our refactoring? Let's use the guidance from a tool called flog to guide our changes.
Flog
Flog describes itself as a tool that:
reports the most tortured code in an easy to read pain report. The higher the score, the more pain the code is in.
You point flog to a file, or directory, and it provides you with a score. The higher the score, the more attention you might want to pay to it. As for how flog calculates the number, I'll let flog summarize itself again:
Flog essentially scores an ABC metric: Assignments, Branches, Calls, with particular attention placed on calls.
Let's look at the flog score of our Guitar class with all the tuning details inside the class:
⇒ flog guitar_with_tuning.rb -a
65.6: flog total
9.4: flog/method average
16.8: Guitar#standard_tuning
16.8: Guitar#down_half_step_tuning
13.0: Guitar#tune
6.6: Guitar#pick
5.2: Guitar#initialize
4.3: Guitar#restring
3.0: Guitar#none
Let's investigate the method with the biggest flog score.
def standard_tuning
@strings[5].tune(note: :e, octave: 2)
@strings[4].tune(note: :a, octave: 2)
@strings[3].tune(note: :d, octave: 3)
@strings[2].tune(note: :g, octave: 3)
@strings[1].tune(note: :b, octave: 3)
@strings[0].tune(note: :e, octave: 4)
end
We know that flog is particularly attuned to looking for calls. Each line in this method has a call to access an element in an array, and then another call to the tune method. That's two calls per line, with six lines in the method.
What do we do with the number?
So, bigger is worse, but how big is bad? At what number should you take action? Thoughtbot's Ruby Science book suggests a method is long or complex with a flog score above 10. It also posits that a class is long or complex with a flog score above 50.
With those heuristics in mind, our Guitar class is long, and all the methods related to tuning are long as well. Let's start by looking at the public method in that class, the method to tune the guitar:
def tune(tuning = :standard)
case tuning
when :standard
standard_tuning
when :down_half_step
down_half_step_tuning
when :drop_d
drop_d_tuning
else
raise "unknown tuning"
end
@tuning = tuning
end
The technique I'd start with to simplify a complex method is to extract smaller methods from it. However, there doesn't seem to be much gained from that here. This happens to be a really long case statement, each branch of which calls another method.
In an attempt to achieve a lower flog score, I'm going to rely on the similar naming of each of these tuning methods. I'll reach for a different tool: metaprogramming.
def tune(tuning = :standard)
raise "unknown tuning" unless VALID_TUNINGS.include?(tuning)
send("#{tuning}_tuning")
@tuning = tuning
end
What does flog think about this change?
⇒ flog guitar_tuning_metaprogramming.rb
58.1: flog total
8.3: flog/method average
5.6: Guitar#tune
The flog score decreased from 13 to 5.6. A clear improvement - in the eyes of the metric. Is it better though? I'd argue that's still a matter of taste. How do you feel about the metaprogramming? How comfortable will you and your team be maintaining this? What pressure does the required naming scheme for any of the tuning methods place on your system? Consider these questions to decide whether to make this change, regardless of the flog score.
Extract Class
Even with this change, we still have a class that has a flog score over 50. That puts it in the "too long" category, according to Ruby Science. So, let's do what we did in the prior post and move everything related to tuning into a Tuner class.
class Tuner
def initialize(guitar)
def tune(tuning = :standard); end
def standard_tuning; end
def down_half_step_tuning; end
def drop_d_tuning; end
def open_a_tuning; end
def modal_c_tuning; end
def all_fourths_tuning; end
def all_fifths_tuning; end
end
We still need to tune our guitar. We achieve that by delegating this responsibility to our Tuner class.
class Guitar
def tune(tuning = :standard)
Tuner.new(self).tune(tuning)
end
end
What does that mean for our flog score?
⇒ flog guitar_separate_tuner.rb
18.9: flog total
3.8: flog/method average
Unsurprisingly, deleting most of the code in a class reduces the complexity of that class. We didn't get rid of the complexity though. We just moved it somewhere else. Let's check the flog score of our new Tuner class.
⇒ flog tuner.rb
58.6: flog total
11.7: flog/method average
25.2: Tuner#standard_tuning
25.2: Tuner#down_half_step_tuning
5.3: Tuner#tune
The complexity of our tuning methods actually got worse, increasing from 16.8 to 25.2. That's because we added another call on each of our lines.
Tuning one string used to look like this:
@strings[5].tune(note: :e, octave: 2)
But now we need to find the string from the guitar that's passed into the tuner.
@guitar.strings[5].tune(note: :e, octave: 2)
Extracting new concepts
To handle the complexity, we're going to reshuffle the responsibility of these ideas. Right now the methods for the different tunings tune each string of the guitar. Instead, let's have the tunings only know what notes to tune the strings to.
We'll also extract each of these tunings out to a separate module.
module StandardTuning
def self.pitches
[
{ note: :e, octave: 4 },
{ note: :b, octave: 3 },
{ note: :g, octave: 3 },
{ note: :d, octave: 3 },
{ note: :a, octave: 2 },
{ note: :e, octave: 2 },
]
end
end
Now when we want to add a new tuning, we add a new module, rather than another method on the Tuner class.
The act of tuning is now isolated into the Tuner#tune method.
class Tuner
def tune(tuning = :standard)
raise "unknown tuning" unless VALID_TUNINGS.include?(tuning)
pitches_for(tuning).each_with_index do |pitch, index|
@guitar.strings[index].tune(**pitch)
end
end
private
def pitches_for(tuning)
Object.const_get(tuning_class_name(tuning)).pitches
end
def tuning_class_name(tuning)
"#{tuning}_tuning".split("_").map(&:capitalize).join
end
end
Keeping with our original refactor, this still uses metaprogramming. This time we use it to recall the appropriate module to get the pitches from.
What does flog think about this?
⇒ flog tuner_without_tunings.rb
21.9: flog total
4.4: flog/method average
8.5: Tuner#tune
6.8: Tuner#tuning_class_name
3.6: Tuner#pitches_for
Our Tuner class has a flog score under 50, and all the methods are under 10. But, like last time, did we just shift the complexity into our extracted tuning modules?
⇒ flog standard_tuning.rb -a
1.5: flog total
1.5: flog/method average
1.5: StandardTuning::pitches
⇒ flog down_half_step_tuning.rb -a
1.5: flog total
1.5: flog/method average
1.5: DownHalfStepTuning::pitches
Each of these is lightweight - a single method returning an array of hashes. According to Ruby Science's heuristics, we no longer have complex methods or classes!
Flog as a forcing function
At the time I didn't refer to flog when making these changes. But it was interesting to reconstruct them with a dedicated focus on their flog scores. When you don't have any intuition around the complexity of a piece of code, it can help as an objective source.
Choosing to lean on some of these changes would require more than only the score. Introducing the metaprogramming would depend on my team's attitude towards that language feature.
Those high flog numbers may have driven me to explore different options. I may not have gotten to the point of building separate modules for the tunings. That freed any given class to know about every different tuning. Paying attention to the flog score pushed me in ways I may not have otherwise considered.
Top comments (0)