Tuesday, March 22, 2016

Interesting Talk: "Code Smells: Your Refactoring Cheat Codes"

I've just watched this great talk by John Pignata: Here is the code of the example shown during the talk.

Kata: Open-Closed Principle in Ruby

Last week some friends from Software Craftsmanship Gran Canaria and I practiced doing Matteo Vaccari's Open-Closed Principle kata.

Jose M. Deniz Suarez and I paired to solve it using Ruby.

We got two versions of the code.

These are the tests:

require_relative '../fizzbuzz'
describe "FizzBuzz" do
it "says same number" do
expect(sayer.say(1)).to eq("1")
expect(sayer.say(2)).to eq("2")
end
it "says Fizz given a multiple of 3" do
expect(sayer.say(3)).to eq("Fizz")
expect(sayer.say(6)).to eq("Fizz")
end
it "says Buzz given a multiple of 5" do
expect(sayer.say(5)).to eq("Buzz")
expect(sayer.say(10)).to eq("Buzz")
end
it "says FizzBuzz given a multiple of 3*5" do
expect(sayer.say(15)).to eq("FizzBuzz")
expect(sayer.say(30)).to eq("FizzBuzz")
end
it "says Bang given a multiple of 7" do
expect(sayer.say(7)).to eq("Bang")
expect(sayer.say(14)).to eq("Bang")
end
it "says FizzBang given a multiple of 3*7" do
expect(sayer.say(3*7)).to eq("FizzBang")
expect(sayer.say(3*7*2)).to eq("FizzBang")
end
it "says BuzzBang given a multiple of 5*7" do
expect(sayer.say(5*7)).to eq("BuzzBang")
expect(sayer.say(5*7*2)).to eq("BuzzBang")
end
it "says FizzBuzzBang given a multiple of 3*5*7" do
expect(sayer.say(3*5*7)).to eq("FizzBuzzBang")
expect(sayer.say(3*5*7*2)).to eq("FizzBuzzBang")
end
def sayer
FizzBuzz.new([
Fizz.new,
Buzz.new,
Bang.new,
SameNumber.new
])
end
end
and the code of the first version:

class FizzBuzz
def initialize sayers
@sayers = sayers
end
def say number
@sayers.reduce("") do |acc, sayer|
sayer.say(number, acc)
end
end
end
class Translator
def say number, acc
return add(number, acc) if predicate(number, acc)
acc
end
end
class SameNumber < Translator
def add number, acc
number.to_s
end
def predicate number, acc
acc == ""
end
end
class Fizz < Translator
def add number, acc
acc + "Fizz"
end
def predicate number, acc
number % 3 == 0
end
end
class Buzz < Translator
def add number, acc
acc + "Buzz"
end
def predicate number, acc
number % 5 == 0
end
end
class Bang < Translator
def add number, acc
acc + "Bang"
end
def predicate number, acc
number % 7 == 0
end
end
In this first version, we used a reduce over a list of translators. Each translator might add or not a translation of the given number to the resulting string.

To try something different from what we've tried other times, we used a template method pattern to remove the duplication between the tranlators.

Even though, we reduced the duplication, we didn't like the solution because the resulting Translator interface (public methods) had parameters that were not used by some of its derived classes.

Next, we deleted the code and try another approach. This time we used the decorator pattern.

The tests were the same except for the sayer factory method:

require_relative '../fizzbuzz'
describe "FizzBuzz" do
# ...
# same tests as in previous version
# ...
def sayer
FizzBuzz.new(Fizz.new(Buzz.new(Bang.new(SameNumber.new))))
end
end
and this the resulting code:

class FizzBuzz
def initialize sayer
@sayer = sayer
end
def say number
@sayer.say(number, "")
end
end
class SameNumber
def say number, acc
return number.to_s if acc == ""
acc
end
end
class Fizz
def initialize sayer
@sayer = sayer
end
def say number, acc
acc += "Fizz" if number % 3 == 0
@sayer.say(number, acc)
end
end
class Buzz
def initialize sayer
@sayer = sayer
end
def say number, acc
acc += "Buzz" if number % 5 == 0
@sayer.say(number, acc)
end
end
class Bang
def initialize sayer
@sayer = sayer
end
def say number, acc
acc += "Bang" if number % 7 == 0
@sayer.say(number, acc)
end
end
Then we refactored it to remove some duplication and added a new translation whose condition didn't have to do with being multiple of some number.

This forcer us to generalize the translators passing them a predicate and a translation string through their constructors.

These are the resulting tests (only showing the new test and the sayer factory):

require_relative '../fizzbuzz'
describe "FizzBuzz" do
# ...
# same tests as in previous versions
# ...
it "says hello given 11" do
expect(sayer.say(11)).to eq("hello")
end
def sayer
FizzBuzz.new(
Translation.new(
"hello",
GivenNumber.new(11),
Translation.new(
"Fizz",
MultipleOf.new(3),
Translation.new(
"Buzz",
MultipleOf.new(5),
Translation.new(
"Bang",
MultipleOf.new(7),
SameNumber.new
)
)
)
)
)
end
end
and code:

https://gist.github.com/trikitrok/619ecabe5ff9f8b08dac

It was a lot of fun.

The constrainsts of this kata are very interesting to practice refactoring and understand the OCP principle.

You can find the code in this GitHub repository.

I'd like to thank Matteo Vaccari for creating it and the great people of Software Craftsmanship Gran Canaria for the fun and the interesting discussions about the different solutions we got.

PS: The "bokata de pata" was also great :)