Robot toy simulator

I have already flunked the test with this code so don't feel bad for cheating any employer. I didn't get any feedback though just wondering what could have gone wrong.

You don't need to get into the functionality, just the overall design. What are short-coming of this code which disqualifies it from being production quality.

Toy Robot Simulator

Description

• The application is a simulation of a toy robot moving on a square tabletop, of dimensions 5 units x 5 units.

• There are no other obstructions on the table surface.

• The robot is free to roam around the surface of the table, but must be prevented from falling to destruction. Any movement that would

result in the robot falling from the table must be prevented,
however further valid movement commands must still be allowed.



Create an application that can read in commands of the following form:

PLACE X,Y,F
MOVE
LEFT
RIGHT
REPORT

• PLACE will put the toy robot on the table in position X,Y and facing NORTH, SOUTH, EAST or WEST.

• The origin (0,0) can be considered to be the SOUTH WEST most corner.

• The first valid command to the robot is a PLACE command, after that, any sequence of commands may be issued, in any order, including

another PLACE command. The application should discard all commands
in the sequence until a valid PLACE command has been executed.

• MOVE will move the toy robot one unit forward in the direction it is currently facing.

• LEFT and RIGHT will rotate the robot 90 degrees in the specified direction without changing the position of the robot.

-
REPORT will announce the X,Y and F of the robot. This can be in any form, but standard output is sufficient.

-
A robot that is not on the table can choose the ignore the MOVE, LEFT, RIGHT and REPORT commands.

• Input can be from a file, or from standard input, as the developer chooses.

• Provide test data to exercise the application.

Constr

Initial impressions

There's a lot of code. Interviewers don't like to read a lot of code.

In many ways, you have overdelivered on the specification. Since you have indicated that this was an interview-question, I'm going to give you my personal value judgements on these points.

-
I'm ambivalent about the logger. On one hand, it is a nice debugging aid. On the other hand, the spec just says that any invalid instruction is to be ignored, so all of those infos, warnings, and errors are unnecessary complications. The spec certainly did not ask for a log/toy_robot.log file to be created by default, so that was a bit of a surprise.

Do you really need to log "Toy Robot created successfully" and "Simulator created successfully" at the "info" level? I'd consider those to be "debug"-level messages, and even then, I'd wonder what could possibly go wrong in such a simple program that the ToyRobot and Simulator would fail to be instantiated.

Instead of logging warnings and errors, you could have just thrown exceptions, catching and reporting them in one exception handler.

• The very first requirement states that the table is a 5 × 5 square. Why are you bothering to accept -x and -y flags to specify the width and height? You could have ditched the entire cli.rb file, accepting no command-line options, and just read from ARGF to satisfy the requirement that the input can come from a file or from standard input.

I'm not convinced that all of those classes need to exist.

• As I mentioned, OptionParser isn't called for, so the CLI class can go away.

• Why is App a separate class from CLI? What about Simulator? Isn't this whole project a simulator app?

• The ToyRobot class looks more like a "dumb" struct rather than a "smart" object.

• The code to support rotations and translations is split up between Direction and Position.

• What is Position#equal? used for? What about Position#to_s — couldn't you have defined it strategically so that it can be reused for the REPORT command?

So, there is some overengineering going on, and some bloat, acting together to produce a solution that is larger than it needs to be.

Second impressions

You do seem familiar with Ruby. For example, you used ranges and === to perform the bounds check instead of 0 <= x && x < @MAX_X && 0 <= y && y < @MAX_Y.

def inside? x,y
   return ((0..@MAX_X-1) === x) && ((0..@MAX_Y-1) === y) 
 end

On the other hand, it would have been slightly prettier with exclusive ranges, and without the explicit return:

def inside?(x, y)
   (0...@MAX_X) === x && (0...@MAX_Y) === y
 end

The Direction enum class is not bad — it demonstrates familiarity with metaprogramming. The attr_accessors would be better as attr_readers, since they should all be constant.

Command dispatching

This is the main point of the exercise.

You've split up the work between App, CommandParser and Simulator. Within the Simulator constructor, there is a command hash that is mostly there to support the PLACE command parameters, and you're repeating yourself by having to list those commands explicitly.

In App#read_command, instead of calling exit, I suggest a more disciplined throw-catch.

Suggested solution

For comparison, this is what I came up with.

class Direction
  def initialize(sym, dx, dy, left, right)
    @name, @dx, @dy, @left, @right = sym.to_s, dx, dy, left, right
  end

  def to_s
    @name
  end

  def left
    Direction.const_get(@left)
  end

  def right
    Direction.const_get(@right)
  end

  def advance(x, y)
    [x + @dx, y + @dy]
  end

  @all = [
    [:EAST,  +1,  0],
    [:NORTH,  0, +1],
    [:WEST,  -1,  0],
    [:SOUTH,  0, -1],
  ]
  @all.each_with_index do |(sym, dx, dy), i|
    self.const_set(sym,
                   Direction.new(sym, dx, dy, @all[(i + 1) % @all.size].first,
                                              @all[(i - 1) % @all.size].first))
  end

  def self.[](name)
    Direction.const_get(name) if Direction.const_defined?(name)
  end
end

class InvalidCommand < Exception ; end

# Mixin for Robot
module Commands
  def place(x, y, face)
    validate(x = (x.to_i if /\A\d+\Z/.match(x)),
             y = (y.to_i if /\A\d+\Z/.match(y)),
             face = Direction[face.upcase])
    @x, @y, @face = x, y, face
  end

  def move
    raise InvalidCommand.new unless @face
    new_x, new_y = @face.advance(@x, @y)
    validate(new_x, new_y, @face)
    @x, @y = new_x, new_y
  end

  def left
    raise InvalidCommand.new unless @face
    @face = @face.left
  end

  def right
    raise InvalidCommand.new unless @face
    @face = @face.right
  end

  def report
    raise InvalidCommand.new unless @face
    @output.puts "#{@x},#{@y},#{@face}"
  end
end

```
class Robot
include Commands

def initialize(board_size=5, output=STDOUT)
@board_size = board_size
@output = output
end

def execute(cmd)
cmd = cmd.strip.downcase

StackExchange Code Review Q#96181, answer score: 8