5
\$\begingroup\$

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
\$\endgroup\$
  • 1
    \$\begingroup\$ I don't have time to look at this now, but want to commend you for an excellent presentation of your question. \$\endgroup\$ – Cary Swoveland Nov 27 '14 at 6:16
  • \$\begingroup\$ @CarySwoveland cheers! I've tinkered with this for several days now, and I've reviewed my own code. \$\endgroup\$ – Mohamad Dec 3 '14 at 12:00
  • \$\begingroup\$ 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. :-) \$\endgroup\$ – Cary Swoveland Dec 4 '14 at 7:21
1
\$\begingroup\$

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!

\$\endgroup\$
  • \$\begingroup\$ 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 []=. \$\endgroup\$ – Mohamad Dec 20 '14 at 18:21
  • \$\begingroup\$ 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. \$\endgroup\$ – Mohamad Dec 23 '14 at 2:24
  • \$\begingroup\$ 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... \$\endgroup\$ – Mohamad Dec 23 '14 at 2:27
  • \$\begingroup\$ 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. \$\endgroup\$ – rui Dec 23 '14 at 23:54
  • \$\begingroup\$ 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). \$\endgroup\$ – rui Dec 23 '14 at 23:56
1
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.