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

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

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

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

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

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

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]

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


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.