General review of Robot class

Question

I'm working alone on my code, trying to learn Ruby as I go.

Class Robot is supposed to be instantiated with the position on a map, and whether it is facing toward a side of the map. It has a couple of moves: left or right, where the face of the robot changes direction, and a move action, one step forward.

class Robot

  def initialize(pos_X, pos_Y, facing)
    @pos_X, @pos_Y, @facing = pos_X, pos_Y, facing
  end

  def move
    world_switch(Proc.new { @pos_X += 1}, Proc.new { @pos_X -= 1},
                 Proc.new { @pos_Y += 1}, Proc.new { @pos_Y -= 1})
  end

  def left
    world_switch(Proc.new {@facing = 'WEST'}, Proc.new {@facing = 'EAST'},
                Proc.new {@facing = 'NORTH'}, Proc.new {@facing = 'SOUTH'})
  end

  def right
    world_switch(Proc.new {@facing = 'EAST'}, Proc.new {@facing = 'WEST'},
                Proc.new {@facing = 'SOUTH'}, Proc.new {@facing = 'NORTH'})
  end

  def report
    puts "Output: " <<  @pos_X.to_s << ',' << @pos_Y.to_s << ',' << @facing
  end

  def world_switch(do_on_north, do_on_south, do_on_east, do_on_west)
    case @facing
    when 'NORTH'
      do_on_north.call
    when 'SOUTH'
      do_on_south.call
    when 'EAST'
      do_on_east.call
    when 'WEST'
      do_on_west.call
    end
  end
end

Answer 1

Looks like you over-thought it a little bit. Callbacks are indeed a powerful abstraction, but it looks overkill in this case. Some notes:

• Use symbols to codify @facing: :east, :north, ...

• a hint to write move without callbacks:

  increments = {:north => [0, +1], :east => [+1, 0], :south => [0, -1], :west => [-1, 0]}
  

• A hint to write left without callbacks:

  new_facing = {:north => :west, :east => :north, :south => :east, :west => :south}
  

This way you describe what the robot does not with code (callbacks) but with data structures (which can be as simple as a hash).

As requested: the complete implementation of left:

def left
  new_facing = {:north => :west, :east => :north, :south => :east, :west => :south}
  @facing = new_facing[@facing]
end

Answer 2

You should definitely apply tokland's suggestions. Additionally:

• Don't use String concatenation (<<, +, +=), use interpolation ("the value is: #{value}") it's more readable, and faster

• Make private methods private, in this case world_switch, but after the refactor, it should already be gone :)

• I'd rename report to to_s this has two advantages:

  • it's the 'natural' method to call when you want a String representation of something
  • it is implicitly called when using String interpolation: "my robot: #{@robot}" # no need for @robot.to_s

• You could also replace report by using @robot.inspect, this will give you the standard String representation for your Robot