Ruby object oriented Snakes and Ladders implementation

Question

I'm not a beginner to programming, but I am to programming in OOP (effectively at least). So to learn better I wrote an object oriented game of Snakes and Ladders. For background, see this question.

I wrote four classes: Board, Cell, Portal, and Player. All are self explanatory except for Portal. A Portal object represents a snake or a ladder cell. Portals are just cells with behaviour. Their behaviour is identical save for the direction they teleport you to. Snakes send you back, while ladders send you forward.

A Cell object keeps track of any players that land on it. It is also aware of its location.

class Cell
 attr_reader :location, :players
 def initialize(input)
 @location = input.fetch(:location)
 @players = []
 end
 def exit(player)
 evict player
 end
 def enter(player, board)
 admit player
 player.set_cell(self)
 end
private
 def admit(player)
 players << player
 end
 def evict(player)
 players.delete(player)
 end
end

A Portal object inherits from Cell. It adds behaviour, and has a destination attribute. It overrides the enter method in its superclass. Instead of admitting the player it tells the board to send it to the portal's destination.

class Portal < Cell
 attr_reader :destination
 def initialize(input)
 @destination = input.fetch(:destination)
 super
 raise ArgumentError, "Location and destination can not be the same" if location.equal?(destination)
 end
 def enter(player, board)
 board.move player, location, destination
 end
end

The Player object must know of the Cell it resides in. It is also responsible for rolling the die.

class Player
 attr_reader :name
 attr_accessor :current_cell
 def initialize(input)
 @name = input.fetch(:name)
 end
 def to_s
 name
 end
 def roll_dice
 rand(1..6)
 end
 def set_cell(cell)
 self.current_cell = cell
 end
 def position
 if current_cell
 current_cell.location
 else
 -1 # represents off the board state
 end
 end
end

Finally, the Board keeps track of the current player, the number of turns, and moves players in between cells. It also decides if the game has a winner.

class Board
 attr_reader :grid, :players, :die
 attr_accessor :turn
 def initialize(input = {})
 @grid = input.fetch(:grid, default_grid)
 @players = []
 @turn = 0
 end
 def add_player(player)
 players << player
 end
 def get_cell(index)
 grid[index]
 end
 def move(player, from, to)
 get_cell(from).exit player if game_started?
 get_cell(to).enter player, self
 end
 def play_turn
 current_player_position = current_player.position
 roll = current_player.roll_dice
 if winner?(roll)
 puts "#{current_player} rolls a #{roll} and wins! Congratulations!"
 exit
 end
 move current_player, current_player.position, current_player.position + roll
 puts "#{current_player} rolls a #{roll} and moves from #{current_player.position - roll} to #{current_player.position}"
 increment_turn
 play_turn
 end
private
 def default_grid
 Array.new(100)
 end
 def current_player
 players.fetch(turn % players.size)
 end
 def increment_turn
 self.turn += 1
 end
 def game_started?
 turn > 0
 end
 def winner?(last_roll)
 # +1 is to account for arrays starting at 0. So cell 1 is in fact cell 2 on a board.
 (current_player.position + 1 + last_roll) >= grid.size
 end
end

Concerns

I think I've done a decent job for my first crack, but I know this can be improved.

1. I am not sure about the die rolling and player moving mechanic. It feels clunky. Is passing the cell to the user a necessary dependency? I can't think of a better way to get the player to move and change its position.
2. The play turn uses recursion, but again, I'm not sure how to write it better.

That arrays in Ruby start at 0 is causing me all sorts of confusion.

1. To render the board I must add 1 to each cell location.
2. I must add 1 to the player position or deduct 1 from the grid size to work out the winner.
3. Players start off the board. I must check if the player has a cell, and if not, I have to return -1 to represent the off the board state.

There are probably lots of issues I'm not aware of. My objectives are the following. I think so far I might have succeeded with 2, but failed with 1 and 3.

1. Make the code easy to modify. Let's say I want to add a Rule object that can store different winning criteria (roll to an excess of 100 or land exactly on 100).
2. Introduce other cell subclasses with different effects.
3. Make the game state easy to save so that I can player across HTTP requests.

Here's an example of running the game:

game = SnakesAndLadders.classic
m = SnakesAndLadders::Player.new name: "Mario", color: "Red"
l = SnakesAndLadders::Player.new name: "Luigi", color: "Green"
game.add_player m
game.add_player l
game.play_turn

• Cary Swoveland: I don't have time to look at this now, but want to commend you for an excellent presentation of your question.
• Mohamad: @CarySwoveland cheers! I've tinkered with this for several days now, and I've reviewed my own code.
• Cary Swoveland: I still haven't found time to look at this, but will try... In the meantime, consider adding some comments that are critical of the answer. :-)

Answer 1

some great work here! I will give you partial implementation that addresses some issues and (hopefully) will give you some idea for further refactoring.

For start a simple wrapper for index/position. It wraps only integer because you use single-dimension world, but you could support multiple dimensions aswell (see comment in code for more detail).

class Location
 attr_reader :x
 def initialize(x)
 @x = x
 end
 # those methods has been overwritten because i want to use different
 # instances of Location as hash keys. As long as @x is the same between
 # two instances of Location it's considered as the same object, i.e.
 # 
 # hash = {}
 # hash[Location.new(1)] = :value
 # hash[Location.new(1)] # => :value
 # if you would want to add pair of coordinates to location you should
 # update those methods to behave the same, i.e.
 #
 # def ==(other); x == other.x && y == other.y;end
 # def hash; [x,y].hash; end
 #
 # hash[Location.new(1,0)] = :value
 # hash[Location.new(1,0)] # => :value
 #
 # This is required only because I use hash as storage
 def ==(other)
 x == other.x
 end
 def eql?(other)
 self.==(other)
 end
 def hash
 x.hash
 end
end

Board looks like hash with some restrictions (size) and has default value of empty cell (Cell). Stores in memory only non-default values. A bit smarter than array in my opinion.

Relying on hash means that the keys can't really be removed and memory won't be freed but it won't be issue if you decide to replace hash with something else as long as it implements contract (methods [] and []=). It can be replaced with some persistent storage (file/db), give it a try! Once you have in-memory and persistent storage you can use hash in tests (it works fast) and file/db board in your web/console application. This is great pattern but very difficult for many people who rely heavily on ActiveRecord. Don't let storage drive your design!

class Board
 def initialize(size=100)
 @size = 100
 @grid = Hash.new(Cell.new)
 end
 # we've added some extra methods to Location, here it pays off.
 def [](location)
 @grid[location]
 end
 def []=(location, value)
 return nil if location.x < 0 || location.x > @size
 @grid[location] = value
 end
end

The cell on board can be entered, that's it. It shares contract (interface) with Snake and Ladder.

class Cell
 def enter(player)
 end
end

Ladder is a bit different, on enter it teleports player. It shares contract with Cell. If you use rspec you should write shared example group for any kind of cell to ensure that it supports enter method.

class Ladder
 def initialize(destination)
 @destination = destination
 end
 def enter(player)
 player.location = @destination
 end
end

For simplicity just copy of Ladder, you could implement your own version to ensure that destination is lower than current location or some other behaviour.

The Portal class didn't really fit into problem domain. Writing code is cheap, reading it later and understanding is not. If something can be done in more expressive way go for it.

Snake = Ladder

Player knows how to move, to get make it work he needs to be aware of the board so we have to inject it. This knowledge could be extracted to some middle object but whatever it's worth it is up to you. Do not try to add too many classes if you don't need it, every class/file you add makes the application more difficult to follow/understand.

I've made it this way because it's simple and demonstrates dependency inversion principle. Board (high level abstration) is being used by Player (lower level abstraction).

There is little risk (or reason) of Board changing it's behaviour, on the other hand Player.move can be adjusted to incorporate some rules or restrictions (i.e. roll 6 three times in a row should invalidate the move), so risk of breaking the application is lower this way.

class Player
 attr_accessor :location, :name
 def initialize(board, name = '')
 @name = name
 @board = board
 @location = Location.new(0)
 end
 def move(num)
 puts "moving #{name} from #{@location.x} to #{@location.x + num}"
 @location = Location.new(@location.x + num)
 @board[@location].enter(self)
 @location.x
 end
end

The Dice.

class Dice
 def self.roll
 1 + rand(6)
 end
end

And finally sample usage (proof of work):

@board = Board.new
@board[Location.new(2)] = Ladder.new(Location.new(5))
@board[Location.new(4)] = Snake.new(Location.new(3))
@player1 = Player.new(@board, 'one')
@player2 = Player.new(@board, 'two')
@player1.move(1) #goes to position 1
@player2.move(2) #enters position 2, there is ladder that teleports him to position 5
loop do
 @player1.move(Dice.roll)
 @player2.move(Dice.roll)
 break if @player1.location.x >= 100 || @player2.location.x >= 100
end

I strongly recommend you to look for presentations of Sandi Metz and get her book (Practical Object-Oriented Design in Ruby (POODR)) - you won't regret it.

Hope you find this useful!

• Mohamad: I saw this and then travelled. Finally now I'm having a chance to sit down and implement it. Some very helpful stuff, here. Thanks! The location class looks a little funky to me, but that's because I'm not used to writing methods with [] and []=.
• Mohamad: Having gone through your solution, I have a couple of questions: What do we gain from the Location wrapper? Why not just use an int like in my refactoring (the hash keys being integers)? I feel like the Location class is also doing some magic. This is probably because I don't fully understand def hash; x.hash; end and how Ruby hashes work. Finally, your version introduces a hard coded Location dependency into Player, admittedly not one that is likely to change. I like your dependency inversion (board to player). I was leaning the same way.
• Mohamad: I also noticed that this is a bit more awkward to test: I'm having a hard time testing the def hash in Location, and player#move. Overall this certainly looks smarter than my implementation, but it makes some compromises. I dislike player.location.x, for example...
• rui: What do we gain from the Location wrapper? Wrapper classes on top of external dependencies (gems, stdlib, even own code sometimes) allow you to cleanly define contract of your object, i.e. there is no point in having all the methods exposed by Integer - you don't want someone to do Location.times do .... It will also allow you to build adapters super easy when you need them.
• rui: I'm having a hard time testing the def hash in Location I wouldn't test #hash, it's implementation detail. From docs: The hash value is used along with eql? by the Hash class to determine if two objects reference the same hash key. What to test in Location class then? that it can be used as hash key: hash[Location.new(1)] == hash[Location.new(1)], that it has same identity across instances Location.new(1) == Location.new(1) and that it accepts x: Location.new(1).x == 1 - that's all you really need and care about. Rule of thumb: test as little as possible (but no less).

Answer 2

With the benefit of several days of thinking I'm going to have a bash and review my own code. To aid my understanding, I made a rough diagram of how my objects interact with each other.

The Portal class remains unchanged, so first up it's Cell. It's decent, but it can be compacted.

1. I removed the admit and evict private methods. They added no value.
2. Ruby 2 introduced keyword arguments. They are superior to an options hash. So I removed that in favour of keywords args in initialize.
3. Player exposes a position setter. Instead of passing self to the player under enter, just set the player's position to the cell's location using the setter.

This refactoring addresses concerns one and two regarding the mechanic. To some extent. More on that later.

class Cell
 attr_reader :location, :players
 def initialize(location:, players: [])
 @location, @players = location, players
 end
 def exit(player)
 players.delete(player)
 end
 def enter(player, board)
 players << player
 player.position = location
 end
end

Next up is the Player class.

1. Because we removed the Cell dependency we can remove the position method in favour of a setter.
2. As with above, I replaced the options hash with keyword arguments.
3. It makes more sense for a player to keep track of its turns. Each time a die is rolled, it is a turn. I created a die_rolls array property to track turns. Its size is the number of turns a player has had.
4. die_rolls removes the need for temp variables to hold onto the current roll in the board class. die_rolls.last always returns it. So I added a convenience method.

 class Player
 attr_reader :name, :color, :die_rolls
 attr_accessor :position
 def initialize(name:, color:, position: 0, die_rolls: [])
 @name, @color, @position, @die_rolls = name, color, position, die_rolls
 end
 def to_s
 name
 end
 def roll_die
 die_rolls.push(rand 1..6).last
 end
 def turns
 die_rolls.size
 end
 def last_roll
 die_rolls.last
 end
 end

Next up it's Board. Board was doing too much. It was keeping track of the players, moving them, keeping track of turns, etc... So I split it into two classes: Board and Game.

First up it's the new Board.

1. It accepts a grid. A grid is just a has to cells and portals. It doesn't care what, and how. As long as it gets a hash, it works.
2. The old move method had a bug. If a player rolled in excess of the board size, it would exit its cell, even though it had no where to go. The new move fixes that. The player only exits if it enters another cell. If no movement happens the method returns nil.

Using a hash instead of an array alleviates the problems of 0 index when using arrays. It makes more sense.

 class Board
 attr_reader :grid
 def initialize(grid: Grid.classic)
 @grid = grid
 end
 def move(player, from, to)
 if destination = get_cell(to)
 if location = get_cell(from)
 location.exit(player)
 end
 destination.enter(player, self)
 end
 end
 def get_cell(index)
 grid[index]
 end
 def size
 grid.size
 end
 end

Finally, it's the Game class. It handles taking turns and deciding the game winner if any.

1. Once more, I've opted for keyword args in initialize.
2. I've simplified the play_turn method. It plays a turn, moves the player, then checks if the player has won (landed on last square). If the player does not move (no where to go), it checks if the last roll gets him in excess of the last square. If either of those things is true, it sets self.winner, otherwise the turn ends.

The rest is self explanatory. over? checks to see if self.winner is not nil. I've isolated dependencies on player and board in their own methods.

The only thing that may not make sense, which I omitted from player class is player.destination_after_last_roll, this is just a convince method for player.last_roll + player.position to check if the last rolls gets it over the winning line since the player can't move off the board.

 class Game
 attr_reader :board, :players, :winner, :turn, :player
 def initialize(board:, players: [], turn: 0)
 @board = board
 @players = players
 @turn = turn
 end
 def play_turn
 return if over?
 init_turn
 puts "#{player} rolls #{player.last_roll}!"
 if board.move(player, player_position, player_destination_after_last_roll)
 self.winner = player if won?
 else
 self.winner = player if will_win?
 end
 end
 def over?
 !!winner
 end
 def simulate
 play_turn until over?
 end
 private
 def init_turn
 @player = players.at(turn % players.size)
 @player.roll_die
 @turn += 1
 end
 def won?
 player_position.equal?(board_size)
 end
 def will_win?
 player_destination_after_last_roll >= board_size
 end
 def winner=(player)
 @winner = player
 puts "Game over! #{winner} wins in #{winner.turns} turns. Congratulations!"
 end
 def player_position
 player.position
 end
 def player_destination_after_last_roll
 player.destination_after_last_roll
 end
 def board_size
 board.size
 end
 end

I've packaged this into a gem, with appropriate tests. I have learned a lot, but still not enough to nail down my perfect design. I still feel that my interfaces are designed poorly.

Here's an example: A rule states that a player who rolls three sixes in a row returns to square one, and may not leave until the player rolls another six.

How would I bake this into my current design with minimal interruption? The natural thing feels like Player should control its own movement. The Player class doesn't know about Board and board.move. So the interface has to be redesigned. There are other issues too. But this is a start.

I will post a follow up.