Skip to content

Instantly share code, notes, and snippets.

What would you like to do?
Refactoring Suggestions

Refactoring Suggestions

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 :)

Indicator: "Feature envy"

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

Indicator: Order dependent methods

Staying 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.

Indicator: Mutation

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.

Idea: function objects

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, 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

  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

  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

  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)

Indicator: Always calling the same method on an object

When 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[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, 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]`

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]

Idea: Make the cards comparable, and have the Turn class only depend on their comparability

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 = :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 = [1,2,7,8], [1,3,9,4]
  assert_equal cards1, [8]
  assert_equal cards2, [4, 1,2,7, 1,3,9]

Okay, 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

  def inspect
    "<#{@value} of #{@suit.capitalize}>"

  include Comparable # defines these methods: <, <=, ==, >=, >
  def <=>(card)      # they are all implemented in terms of this method, "the spaceship operator"
    value <=> card.value

five = 5, :hearts
six  = 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>]

Anticipated Indicator: wanting more than one public method

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 winner method 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.

Okay, good luck :P

Feel free to ping me if anything I've said is unclear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment