Hey, this morning I was thinking about the code we looked at yesterday, and how I'd refactor it. I came up with a few ideas, generally with overlapping reasons for the changes. I'm on a long train ride to St. Louis, and figured I'd write up what things I noticed and how I would make changes based on those things. It got a little long, b/c I had a few goals with it:
- I wanted to make sure I explained my reasoning so that it wasn't just a religious "this is better than that b/c I said so".
- I wanted to make sure I explained what I was noticing that led me to these ideas and conclusions, so that you would know how to identify them yourself.
- I wanted to make sure my explanations were concrete enough to be understood, so I put code samples in wherever I felt they would be clarifying.
Anyway, feel free to use as much of it as you like and ignore as much as you dislike :)
When one class is using all the features of another
class, this indicates an issue. Sometimes it implies that whatever the first class is doing actually belongs
in the second class. We saw this when the test knew all the details of the Turn class. It was interested in
the turn class's type, who the winner was, the method that moved the cards into
the @spoils_of_war, and the one that moved them from the @spoils_of_war into the winner's hand.
You recognized towards the end that
maybe this logic belonged in the turn class instead of the test, and I think
that's a good recognition. So perhaps there is a method that does the conditional
check on the type and winner and then calls the other two methods, allowing the
details that the test knew about to move into the turn class.
In our case, that method would be the method that "takes the turn", so perhaps
we call it take. However, the Ruby community has largely converged on using
the method name call for "the method that does the thing that this object is here to do".
And I've taken to naming classes with a #call method as verbs. So perhaps
rename Turn to TakeTurn, and then add a #call method which does something
like (sorry, I was pretty tired at EOD yesterday and don't remember the method
names, so my names will be a little off, but the idea should still make sense):
class TakeTurn
def call
# Note that this is creating a local variable named `winner`, whose value
# is whatever the `winner` method returns. We're calling the method with an
# explicit `self` so that Ruby understands we want it to call the method and not
# assign the variable to itself.
#
# We're doing this first b/c I think it may return the wrong answer after the
# spoils of ware are removed from the players' cards (IOW, may be order dependent)
winner = self.winner
extract_spoils_of_war
assign_cards(winner)
end
endStaying on the idea above, the method that
moved the cards into the spoils of war had to be called before winner was called
or it wouldn't have the @spoils_of_war to place back into the winner's deck.
This implies to me that these methods should be merged, or at least look that
way from the outside (note that you can use Module#private to hide methods from the
outside world so that you can keep things separated without breaking encapsulation).
Then the test would call only one method, which would perform this operation
without exposing a way to be used incorrectly. This winds up indicating to us
the same thing that the feature envy indicated.
Modifying data can make sense for performance optimization,
but it's generally less maintainable. Eg if nothing is ever mutated, then it's
not possible to have order dependent methods. My general rule of thumb is that,
unless I have a specific reason to, I don't modify any argument passed in to my
methods, I only modify data that my method or class has created or asked other
things to create. This is probably not true for data structures (eg if you wanted
to create a queue or stack or something), but it is true for classes that perform
operations / algorithms, like your Turn class. Also note that this is the reason
why people prefer attr_reader over attr_accessor, because exposing the setter
method implies that the class expects the outside world to mutate it. Potentially
acceptable for classes that are pure data (though many people avoid it here,
as well, creating what they call "value objects" instead), but avoid it for algorithms.
So, in our case, Turn has two methods which the outside world can use to modify
its internal state (the @spoils_of_war). Note that if we had no mutation, then
it wouldn't have been possible to make order dependent methods, so this reinforces
what we noticed above and again implies to us that we should merge these methods
together so that the outside world doesn't have an obvious way.
We also mutate the player's cards in both of those methods. Given that I avoid
mutation, I would avoid this. As an example of the pain that it can cause, it
potentially makes the winner method incorrect, because it probably checks the
first card in the player's hand, and these methods are going to change the player's
hand, thus the need to save the winner local variable in my code example above.
(I didn't look at this method very closely, so perhaps there is no issue, but
it is something that we have to worry about, because it feasibly could be an issue).
This leads me to thinking about ways to avoid the mutation in those two methods.
When a class performs an algorithm like the Turn class
does, I've found it best to think about it like a function. A function is something
that you invoke, giving it the data it needs to do its job. Depending on who you
talk to, a function will also not modify any state that is visible to the outside
world, and will instead return data. So in our case, if our Turn class was a
function, instead of modifying player1 and player2, perhaps it would return
copies of those players, with their cards set to the new values that they should be.
This would again work well with avoiding mutation and would avoid order dependent
methods because the only thing you can really do with a function is call it, so
it gets away from order dependent methods.
There are any number of ways to do this, I could talk about this for a really long time, but here is a pretty reasonable general purpose way to structure these:
class DoSomething # class is a verb
def self.call(arg1, arg2) # the world calls this method
# if it's simple enough, you can put it all in this `.call` method
new(arg1, arg2).call # or you can delegate to a new instance, like this
end
def initialize(arg1, arg2) # args are passed to initialize instead of call
self.arg1 = arg1 # save args using setters to catch misspellings
self.arg2 = arg2
end
def call # the only public method, it does the thing the class is here to do
# ... # note that I generally try to restrict access to instance variables / getters to just this method
end
private # hides implementation details used by `call` so that you can change the implementation without breaking callers
attr_accessor :arg1, :arg2 # we use setters/getters b/c spelling errors raise exceptions, and make them private to maintain encapsulation
def helper_method(arg) # generally prefer to receive things these methods need as args instead of talking directly to getters/ivars
# ... # default to not mutating in these methods (you can violate all these rules, but make sure you have a reason... most obvious reason is probably memoization)
end
endWhen a piece of code is
always calling the same method on an object, that indicates that it's dependencies
are wrong. In our case, the Turn class depends on player1, and player2, but
IIRC, it always asked for those player's cards. Does it use anything on those players
other than their cards? If so, then that indicates that we should be passing in
the players' cards instead of the players.
If this is the case, then a really nice thing results from this: it makes mutation
avoidance much easier. Eg there are many methods (such as Enumerable#take and Enumerable#drop)
which will work on the arrays, and avoid mutation. Or, if it is easiest to implement
through mutation, we can duplicate the array (Array#dup) and then mutate the
duplicated array instead of the one that we were provided. Then the winner method
will be simpler (b/c it can say cards1[0] instead of player1.cards[0], which
I assume it does), and it won't be at risk of becoming incorrect due to mutation.
This will also make your tests easier to write, because you won't have to create
the Player objects anymore, since Turn won't depend on them anymore.
So then our TakeTurn#call method would return the two new sets of cards as an array,
which can then be assigned positionally back into the original variables or new
variables, depending on what is most useful. eg:
# This defines `call` on `self` (the main object), I can explain how it does this if you really understand that object/class/binding structure I sent you.
def self.call(cards1, cards2)
new_cards1 = cards1.drop 1 # makes new array instead of mutating
new_cards2 = cards2.drop 1
winner = new_cards2 # a fake implementation of winner determination
winner.concat cards1.take(1) # pragmatically, we allow mutation on objects created in this method
winner.concat cards2.take(1)
return new_cards1, new_cards2 # equivalent to returning `[new_cards1, new_cards2]`
end
cards1 = [1,3,5]
cards2 = [2,4,6]
new_cards1, new_cards2 = call(cards1, cards2)
# Noting was modified
cards1 # => [1, 3, 5]
cards2 # => [2, 4, 6]
# and we have access to results
new_cards1 # => [3, 5]
new_cards2 # => [4, 6, 1, 2]Right now, the tests are painful to write, because you have to create all the cards
to put into the players hands. Eg there were 8 lines like card1 = Card.new :hearts, 1 or something.
This was so that you could set the player's cards to [card1, card2, card7, card8] or something.
However, if the only thing that mattered was that you could do card1 < card2, then
you could use any comparable object for the cards when testing the Turn class.
In particular:
def test_equal_value_leads_to_war
cards1, cards2 = TakeTurn.call [1,2,7,8], [1,3,9,4]
assert_equal cards1, [8]
assert_equal cards2, [4, 1,2,7, 1,3,9]
endOkay, so how do we make them comparable? We need to implement a method named <=>,
which returns a negative value if the receiver is less than the argument, a positive
value if the receiver is greater than the argument, and 0 if they are equal.
You can see docs on this by running ri Comparable from the shell.
class Card
attr_reader :value
def initialize(value, suit)
@value = value
@suit = suit
end
def inspect
"<#{@value} of #{@suit.capitalize}>"
end
include Comparable # defines these methods: <, <=, ==, >=, >
def <=>(card) # they are all implemented in terms of this method, "the spaceship operator"
value <=> card.value
end
end
five = Card.new 5, :hearts
six = Card.new 6, :spades
five <=> six # => -1
five < six # => true
five > six # => false
[five, six].sort # => [<5 of Hearts>, <6 of Spades>]
[six, five].sort # => [<5 of Hearts>, <6 of Spades>]If you implement the recommendations I made here, then you may hit an issue where you want to know who won, so that you can report this to the players. However, I've suggested that this method be made private.
I can think of 3 potential answers here:
- Return a third value: which set of cards won
- Make the
winnermethod public (acceptable if it doesn't modify data, but undermines the idea that this class is a function) - Extract that method into its own function object so you can call it from more than one place.
Feel free to ping me if anything I've said is unclear.