Making my Ruby 'Legacy Code' SOLID

Several weeks ago I was working on a TicTacToe game in Ruby, but then I moved on to Java for a while. Now I've returned, full of type-security and compiler error handling, and hopefully some transferrable things too. There's plenty to look at, but I'll just be working through a single example for each SOLID principle.

Dependency Inversion

I'm starting with the 'D' in SOLID because when I came back to this project I noticed how tightly coupled the whole thing was, making any change very difficult. Most classes instantiated others instead of having them injected, so that was the first thing I changed.

Take a look at this test for my Game class. It makes an instance of Game, refers to both Board and ConsoleInOut, and fixes the gets result of ConsoleInOut.

Before

require 'game'

describe Game do
    it 'Asks for board size input' do
        allow_any_instance_of(ConsoleInOut).to receive(:gets).and_return('4')
        game = Game.new
        expect(game.board.board_size).to eq 4
    end
end

How do we know that Game contains instances of these classes? How do we change their input and output? We don't and we can't (unless you count allow_any_instance_of as changing input).
We have an opaque bundle of prima-donna classes that insist the tests be written around them. Let's change that by instantiating all of Game's dependencies externally and then injecting them on instantiation.

After

require 'game'

describe Game do
    before(:each) do
        @inOut = ConsoleInOut.new('', '')
        @player_x = Player.new('Player 1', 'X')
        @player_o = Player.new('Player 2', 'O')
    end

    it 'Asks for board size input' do
        board = Board.new(@inOut, 4)
        game = Game.new(@inOut, @player_x, @player_o, board)
        expect(game.board.board_size).to eq 4
    end
end

Now we know exactly what Game contains, because we passed them in, and this allows us to change the parameters of these classes beforehand. For example, when we pass in a ConsoleInOut instance, we can replace STDIN with a string to simulate input, and STDOUT with a string so that the test output isn't polluted by every print method.

Single Responsibility

Classes shouldn't have more than one responsibility, plain and simple. There are more technical explanations out there, but it's enough so say that my ConsoleInOut class is both getting input and validating it.

It works, but if I were to swap out ConsoleInOut for, say, WebInOut, I'd need to reimplement validation as well as Input/Output.

Since Game will always get the same input, there's no reason to ever change validation, so let's go ahead and put it in the Game class that handles interactions between classes.

Before

class ConsoleInOut
    def initialize(input, output)
        @input = input
        @output = output
    end

    def get_input
        #
    end

    def print(to_print)
        #
    end        

    def select_move(max_turns)
        print("Please enter a free number from 1-#{max_turns}")
        player_input = gets.chomp.to_i
        valid_player_input(player_input, max_turns)
    end

    def valid_player_input(player_input, max_turns)
        player_input >= 1 && player_input <= max_turns ? player_input : 
        select_move(max_turns)
    end

    # And some similar select/validate methods
end

class Game

    # And the rest of the Game

    def submit_move
        player_input = @inOut.select_move(@board.max_turns)
        @board.make_move(current_player.mark, player_input - 1)
        @board.view_board
    end
end

After

class ConsoleInOut
    def select_move
        player_input = gets.chomp.to_i
    end

    # Remove valid_player_input, rest is the same
end

class Game

    # The rest is the same

    def submit_move
        valid_move = false
        until(valid_move)
            @inOut.print("Please enter a free number from 1-#{@board.max_turns}\n")
            player_input = @inOut.select_move(@board.max_turns)
            valid_move = @board.is_square_free?(player_input - 1)
        end
        @board.make_move(current_player.mark, player_input - 1)
        @board.view_board
    end
end

Now ConsoleInOut just gets input for Game, without knowing or caring what happens next. Game is now a bit bigger, but more complete, and validation is functionality that shouldn't ever need changing. As a bonus, if anything goes wrong with input and validation, it's easier to place the blame.

Open/Closed

I'm going to be very daring and use a metaphor now:

• A gaming console or desktop computer is complete in itself.

• It doesn't need a particular screen to work, just a screen, and the HDMI port lets you plug it into a huge range of screens without changing anything else.

• If one screen breaks, the console still works, just find a new screen.

• But if you took out the port and built-in a screen, it would be less modifiable, and the screen breaking would be serious.

Your code should have more 'ports' and fewer 'built-ins'. Metaphor over, promise that's the only one.

So...

In the case of a program, it should be be complete and closed to changes, yet open for new features to be added.

If I were to add a Computer Player to my game, it would require rewriting the game so that the Computer could choose to make a move without console input.

In order to make the program open to extension, it is necessary to create a seam by moving the responsibility of selecting a move to the Player class. Then, the core program will not require changes.

Take the code above as a 'before' example for Game.

Before

class Player
    attr_reader :id, :mark
    def initialize(id, mark)
        @id = id
        @mark = mark
    end
end

After

class HumanPlayer
    attr_reader :id, :mark
    def initialize(id, mark, inOut)
        @inOut = inOut
        @id = id
        @mark = mark
    end

    def make_move
        @inOut.select_move
    end
end

class Game

    # The rest is the same

    def submit_move
        valid_move = false
        until(valid_move)
            @inOut.print("Please enter a free number from 1-#{@board.max_turns}\n")
----->      player_input = current_player.make_move
            valid_move = @board.is_square_free?(player_input - 1)
        end
        @board.make_move(current_player.mark, player_input - 1)
        @board.view_board
    end
end

Now that the instance referred to by current_player is responsible for returning a move, we can just swap out a now Human_Player class to extend the program with a Computer player. See below.

Liskov Substitution

In Java, you can make sure that one class is substitutable for a similar one with interfaces or abstract classes. Ruby doesn't have that, but as a colleague is fond of saying, "we can keep the interface in our heads". An interface contains a list of methods that a class doing this particular job must have, but leaves the implementation up to each class. So instead of having an interface file and having the compiler enforce it, we will just imagine that that's going on.

See the HumanPlayer class and its tests below to see what we're working with.

Human Player

class HumanPlayer
    attr_reader :id, :mark
    def initialize(id, mark, inOut)
        @inOut = inOut
        @id = id
        @mark = mark
    end

    def make_move
        @inOut.select_move
    end
end

Tests

require 'human_player'
require 'console_in_out'

describe HumanPlayer do

    before(:each) do
        inOut = ConsoleMock.new('3', '')
        @player = HumanPlayer.new('Player 1', 'X', inOut)
    end

    it 'Stores player name' do
        expect(@player.id).to eq('Player 1')
    end

    it 'Stores player name' do
        expect(@player.mark).to eq('X')
    end

    it 'Can make a move' do
        expect(@player.make_move(9)).to eq(3)
    end
end

The HumanPlayer class stores information (id and mark), and has a make_move method. If this class implemented an interface, that interface might look like this.

Player Interface

interface Player
    def initialize(id, mark)
    end

    def make_move(max_turns)
    end
end

With the same methods, but no body, you can implement the functionality however suits you, and replace a generic Player instance with any instance that fits this pattern.

I've also added max_turns to make_move as I'm anticipating the computer using the rand method in the initial implementation, so HumanPlayer needs this to make the classes substitutable.

Note: It's worth passing in an instance of the Board class so that the computer has knowledge of the Board state, but for the sake of this example I'm keeping it short.

Computer Player

class ComputerPlayer
    attr_reader :id, :mark
    def initialize
        @id = 'Computer'
        @mark = 'O'
    end

    def make_move(max_turns)
        rand(max_turns)
    end
end

To summarise, it doesn't matter how Player or ComputerPlayer work, just that they contain the same core methods. Want to add methods to one or the other? Fine, they will still conform to the Liskov Substitution Principle as long as they both have make_move and store id and mark, since other classes refer to these.

If I want to change how HumanPlayer or ComputerPlayer makes a move, I can do this without modifying Game, and I can add a third option without changing anything.

We're nearly done.

Interface Segregation

I don't have a specific code example for this principle, as this is a small program with two (imaginary) interfaces. The 'imaginary' is important; Ruby doesn't have interfaces, so if the idea is still unfamiliar to you, feel free to move on to the next section.
Otherwise, I will briefly cover the idea behind the Interface Segregation Principle.

The Interface Segregation Principle states that more, smaller interfaces are better than fewer, larger interfaces. This is to prevent the forced implementation of unused methods by a client. In the case that the In/Out interface grew too large, it may be possible that a class implementing it didn't use all of its methods, but had to add them anyway to avoid compiler errors. Why should you add code that isn't used? You shouldn't, meaning this bloated interface should be broken up so that each class only contains methods it uses.

It's also helpful to have smaller classes and interfaces to follow the Open/Closed Principle mentioned earlier; with fewer modifications necessary and more seams available, any future change is much easier.

It's All Connected

Interestingly there's overlap between the SOLID principles, so each of these improvements doesn't just improve the program in one way; having a Liskov Subsitution approved In/Out interface makes dependencies more manageable, as well as making the program open to extension. Furthermore, exporting make_move functionality to a Player better follows the Open/Closed Principle, narrows the responsibility of Game, and paves the way for a Liskov Substitutable computer_player class.