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

tokland · Accepted Answer · 2012-10-24 07:37:24Z

3

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

edited Oct 24, 2012 at 7:37

answered Oct 3, 2012 at 8:34

tokland

11.2k1 gold badge21 silver badges26 bronze badges

\$\begingroup\$ +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/… \$\endgroup\$

Jackie Chan
– Jackie Chan

2012-10-03 08:51:01 +00:00
Commented Oct 3, 2012 at 8:51

Add a comment |

David Österreicher · Accepted Answer · 2012-10-25 00:46:40Z

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

Stack Exchange Network

General review of Robot class

2 Answers 2

You must log in to answer this question.

Linked

Hot Network Questions

General review of Robot class

2 Answers 2

You must log in to answer this question.

Linked

Related

Hot Network Questions