Ruby. Code review. Working alone on the code

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

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

+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/…

David Österreicher · Answer 2 · 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

asked	9 months ago
viewed	129 times
active	8 months ago

Ruby. Code review. Working alone on the code

2 Answers

Your Answer

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

Linked

Ruby. Code review. Working alone on the code

2 Answers

Your Answer

Sign up or log in

Post as a guest

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

Linked

Related