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

• Jackie Chan: +1 to write the code without callbacks (in this case def) But can't understand how 'new_facing' will be implemented, please look at the implementation file posted later: codereview.stackexchange.com/questions/16109/…

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