In reviewing some code for my Rock, Paper, Scissors program, I noticed a recurring Ruby Reek smell, Utility Function that suggested I might want to refactor. Here are the steps I took for future reference.
The Starting Code
Here’s what some of the code I was working on looked like when I started (source):
Move Class
1
2
3
4
5
6
7
8
9
10
11
class Move
attr_reader :name, :winning_moves
def >(other_move)
winning_moves.include?(other_move.name)
end
def to_s
name
end
end
Rock Class
1
2
3
4
5
6
7
8
9
10
class Rock < Move
def initialize
@name = 'rock'
@winning_moves = ['scissors', 'lizard']
end
def win_message(other)
"Rock crushes #{other}."
end
end
Scissors Class
1
2
3
4
5
6
7
8
9
10
11
12
13
14
class Scissors < Move
def initialize
@name = 'scissors'
@winning_moves = ['paper', 'lizard']
end
def win_message(other)
if other.name == 'paper'
"Scissors cuts paper."
else
"Scissors decapitates lizard."
end
end
end
I had three other classes similar to the Scissors class that I’m not showing here.
This isn’t too bad as it is. The Rock class doesn’t raise any Rubocop or Ruby Reek warnings. The Scissors class and the three remaining classes, on the other hand, raised the Ruby Reek Utility Function smell for each win_message method.
1
Scissors#win_message doesn't depend on instance state (maybe move it to another class?) [UtilityFunction]
This basically means that the String returned from the if statement branches does not have any relationship to the rest of the class. That is, the string doesn’t depend on the state of the class or on anything else for that matter. It’s just a string with no connection to anything.
I think this is a sign that the code might make more sense in a different class or module.
Refactoring
I wasn’t sure what to do with this. I’ve seen this utility function smell many times before and at my current level, I often end up disabling the smell and ignoring it. So, I started making small changes in hopes that I might see a pattern that I hadn’t seen previously.
The First Change
The first change I made was to make the strings dependent on the moves:
1
2
3
4
5
6
7
def win_message(other)
if other.name == 'paper'
"#{name.capitalize} cuts #{other}."
else
"#{name.capitalize} decapitates #{other}."
end
end
That worked and successfully stopped the utility function warning. However, it introduced another Ruby Reek smell, the Duplicate Method Call smell.
1
win_message calls 'name.capitalize' 2 times [DuplicateMethodCall]
The Second Change
I’m starting to see a pattern. I’m still not sure how I’m going to determine which verb (cuts and decapitates in this case) to use. Using a hash came to mind, so I tried that.
1
2
3
4
5
6
7
8
9
10
11
12
13
14
class Scissors < Move
attr_reader :win_messages
def initialize
@name = 'scissors'
@winning_moves = ['paper', 'lizard']
@win_messages = { 'paper' => 'Scissors cuts paper.',
'lizard' => 'Scissors decapitates lizard' }
end
def win_message(other)
win_messages[other.name]
end
end
Great! No more Ruby Reek warnings. This code does look a lot cleaner, I had to admit. However, there’s still some room for improvement and it’s at this point where I see an even broader pattern. What if I combine the two instance variables into one instance variable using the hash instead of the array?
1
2
3
4
5
6
7
8
9
10
11
class Scissors < Move
def initialize
@name = 'scissors'
@winning_moves = { 'paper' => 'Scissors cuts paper.',
'lizard' => 'Scissors decapitates lizard' }
end
def win_message(other)
winning_moves[other.name]
end
end
That works, and it’s even cleaner.
The Third and Final Change
That’s when I realized that with some more tweaking, I could move the win_message instance method into the Move superclass, since at this point, it’s going to be the same code for each subclass. Yet, doing so, created the problem I had before where I would not know when to use which verb. To solve this, I realized that I didn’t need to store the entire sentence in the @winning_moves hash, only the verb itself.
At the end, my classes looked like this:
Move Class
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
class Move
attr_reader :name, :winning_moves
def >(other_move)
winning_moves.key?(other_move.name)
end
def win_message(other)
"#{self} #{winning_moves[other.name]} #{other}."
end
def to_s
name.capitalize
end
end
Rock Class
1
2
3
4
5
6
7
class Rock < Move
def initialize
@name = 'rock'
@winning_moves = { 'scissors' => 'crushes',
'lizard' => 'crushes' }
end
end
Scissors Class
1
2
3
4
5
6
7
class Scissors < Move
def initialize
@name = 'scissors'
@winning_moves = { 'paper' => "cuts",
'lizard' => "decapitates" }
end
end
So, since each subclass inherits the methods (behaviors) from the superclass, each instance of a subclass will use those methods on its unique state. In the >(other_move) method, I used key? instead of include? because even though both work, I think key? is more explicit in terms of what the code is looking for. By the time the win_message is called, a winner has been determined and the desired @winning_moves instance variable is available. I also made the default output for the to_s method capitalized since the move name needs to be capitalized whenever it’s used.
Summary
The code looks much cleaner now and I’m no longer relying on if/then statements within each subclass to determine which String to return. This also gives me more flexibility to make changes to the wording or changes to the verbs in the future.
In looking at each subclass, I feel like there may be some way to condense this further, like using a YAML file or something like that to store the names of the moves, the winning moves, and their verbs. Then, I can have a method that plugs those in as needed.
For now, what I have will work.