Robot toy simulator

Question

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.

Constraints

• The toy robot must not fall off the table during movement. This also includes the initial placement of the toy robot.
• Any move that would cause the robot to fall must be ignored.

Example Input and Output

Example a

PLACE 0,0,NORTH
MOVE
REPORT

Expected output:

0,1,NORTH

Example b

PLACE 0,0,NORTH
LEFT
REPORT

Expected output:

0,0,WEST

Example c

PLACE 1,2,EAST
MOVE
MOVE
LEFT
MOVE
REPORT

Expected output

3,3,NORTH

Deliverables

The Ruby source files, the test data and any test code.

It is not required to provide any graphical output showing the movement of the toy robot.

GitHub (Please refer to see the whole project)

cli.rb

module Robot
 class CLI
 def initialize
 options = {:file => nil , :x => 5 , :y => 5}
 parser = OptionParser.new do|opts|
 opts.banner = "Usage: toyrobot [options]"
 opts.on('-f', '--file filepath', 'Filepath for input commands') do |filename|
 options[:file] = filename
 end
 opts.on('-x', '--xcoordinate X', 'Max X co-ordinate(Number)') do |max_x|
 begin
 options[:x] = Integer(max_x)
 rescue
 puts "Invalid x argument"
 puts opts
 exit
 end 
 end
 opts.on('-y', '--ycoordinate Y', 'Max Y co-ordinate(Number)') do |max_y|
 begin
 options[:y] = Integer(max_y)
 rescue
 puts "Invalid y argument"
 puts opts
 exit
 end 
 end
 opts.on('-h', '--help', 'Displays Help') do
 puts opts
 exit
 end
 end 
 parser.parse!
 @application = Robot::App.new(options)
 end
 def start
 @application.start
 end
 end
end

app.rb

module Robot
 class App
 def initialize(opts)
 @input_file = opts[:file].nil? ? STDIN : File.open(opts[:file]) 
 @simulator = Robot::Simulator.new opts[:x], opts[:y]
 end
 def start 
 command = read_command
 while (command) do 
 $logger.debug("Received command #{command}")
 begin
 @simulator.execute command
 rescue => e
 $logger.error(e)
 end
 command = read_command
 end
 end
 def read_command
 print "# " if @input_file == STDIN
 command = @input_file.gets
 exit if command.nil? || (command.chomp.downcase == ".quit")
 command 
 end
 end
end

command_parser.rb

module Robot
 class CommandParser
 @@number_reg = /^\d+$/
 # allowed_commnads is a hash 
 # with value as :method => [[arg types],[regex_range_for_strings_only]]
 def initialize(allowed_commands)
 @allowed_commands = allowed_commands
 $logger.info("Allowed commands are #{@allowed_commands.keys}")
 end
 def parse command
 $logger.debug("Parsing command #{command}") 
 args = command.split " "
 method = args.delete_at(0)
 if valid?(method, args) 
 update_args! method , args
 yield method , args
 return true
 else
 $logger.warn("Parsing failed. Invalid #{command}")
 return false
 end
 end
 private
 def update_args! method , args 
 @allowed_commands[method][0].each_with_index do |arg_type,i|
 case arg_type
 when :number
 args[i] = args[i].to_i 
 when :string
 end
 end
 end
 def valid? (method , args) 
 return false unless @allowed_commands.has_key? method
 unless @allowed_commands[method].nil?
 return false unless @allowed_commands[method][0].size == args.size
 @allowed_commands[method][0].each_with_index do |arg_type,i|
 case arg_type
 when :number
 return false unless args[i] =~ @@number_reg
 when :string
 allowed_reg = @allowed_commands[method][1][i]
 unless allowed_reg.nil?
 return false unless args[i] =~ /#{allowed_reg}/
 end
 end 
 end
 end
 return true
 end
 end
end

direction.rb

module Robot
 class Direction
 attr_accessor :direction
 def initialize(direction)
 @direction = direction
 end
 @NORTH = Direction.new "NORTH"
 @SOUTH = Direction.new "SOUTH"
 @EAST = Direction.new "EAST"
 @WEST = Direction.new "WEST"
 @CLOCKWISE_DIRECTIONS = [@NORTH,@EAST,@SOUTH,@WEST]
 def to_s
 @direction
 end
 class << self
 attr_accessor :NORTH, :SOUTH, :EAST, :WEST 
 end
 def self.find(direction)
 @CLOCKWISE_DIRECTIONS.find{|d| d.direction == direction.upcase }
 end
 def self.left(direction)
 @CLOCKWISE_DIRECTIONS[( @CLOCKWISE_DIRECTIONS.index(direction) - 1 ) % 4] 
 end
 def self.right(direction)
 @CLOCKWISE_DIRECTIONS[( @CLOCKWISE_DIRECTIONS.index(direction) + 1 ) % 4] 
 end
 end
end

position.rb

module Robot
 class Position
 attr_accessor :x, :y, :direction
 def initialize(x,y,direction)
 @x = x
 @y = y
 @direction = direction
 end
 def to_s
 "X= #{@x} Y=#{@y} Facing=#{@direction.to_s}"
 end
 def equal?(another_position)
 self.x == another_position.x &&
 self.y == another_position.y &&
 self.direction == another_position.direction
 end
 def move 
 curr_postion = self.dup
 case curr_postion.direction
 when Direction.NORTH
 curr_postion.y +=1 
 when Direction.SOUTH
 curr_postion.y -=1 
 when Direction.EAST
 curr_postion.x +=1 
 when Direction.WEST
 curr_postion.x -=1 
 end
 curr_postion
 end
 def left
 curr_postion = self.dup
 curr_postion.direction = Direction.left @direction
 curr_postion
 end
 def right
 curr_postion = self.dup
 curr_postion.direction = Direction.right @direction
 curr_postion
 end
 end
end

simulator.rb

module Robot
 class Simulator
 attr_accessor :toy_robot
 def initialize max_x, max_y
 commands = {
 "PLACE" => [
 [:number , :number , :string],
 [nil,nil,"^NORTH$|^SOUTH$|^EAST$|^WEST$"]
 ], 
 "MOVE" => [[],[]], 
 "LEFT" => [[],[]],
 "RIGHT" => [[],[]], 
 "REPORT" => [[],[]] 
 }
 @command_parser = CommandParser.new(commands)
 @table = Table.new max_x , max_y
 @toy_robot = ToyRobot.new
 $logger.info "Simulator created successfully."
 end
 def execute(command)
 r = @command_parser.parse(command) do |method,args|
 $logger.debug("#{method.downcase} - args #{args}")
 self.send( method.downcase , * args)
 end
 $logger.debug(@toy_robot.to_s)
 end
 def place x , y , face
 if @table.inside?(x, y)
 @toy_robot.position = Position.new(x, y, Direction.find(face))
 @toy_robot.placed = true
 end
 end
 def move
 return unless @toy_robot.placed 
 next_position = @toy_robot.position.move
 if @table.inside? next_position.x , next_position.y
 @toy_robot.position = next_position
 else
 ignore
 end
 end
 def left
 return unless @toy_robot.placed 
 @toy_robot.position = @toy_robot.position.left 
 end
 def right
 return unless @toy_robot.placed
 @toy_robot.position = @toy_robot.position.right
 end
 def report
 if @toy_robot.placed
 puts "#{@toy_robot.position.x} #{@toy_robot.position.y} #{@toy_robot.position.direction}"
 else
 puts "Robot is not placed yet. Please use PLACE command to place the robot."
 end
 end
 def ignore
 $logger.debug "Ignored step towards #{toy_robot.position.direction}"
 end
 end
end

table.rb

module Robot
 class Table
 def initialize max_x , max_y
 @MAX_X = max_x
 @MAX_Y = max_y
 $logger.info "Table boundaries are #{@MAX_X},#{@MAX_Y}"
 end
 def inside? x,y
 return ((0..@MAX_X-1) === x) && ((0..@MAX_Y-1) === y) 
 end
 end
end

toyrobot.rb

module Robot
 class ToyRobot
 attr_accessor :position, :placed
 def initialize
 @position = nil
 @placed = false
 $logger.info "Toy Robot created successfully."
 end
 def to_s
 if @placed 
 "Placed at #{@position.to_s}"
 else 
 "Not placed"
 end
 end
 end
end

robot.rb

$LOAD_PATH << File.join(File.dirname(__FILE__))
require "optparse"
require "logger" 
require "robot/version"
require 'robot/command_parser'
require 'robot/table'
require 'robot/position'
require 'robot/toy_robot'
require "robot/direction"
require "robot/simulator"
require "robot/app"
require "robot/cli"
$logger = Logger.new('log/toy_robot.log')

Test code

command_parser_spec.rb

require_relative '../spec_helper'
require_relative '../../lib/robot'
include Robot
describe CommandParser do
 subject(:command_parser) {CommandParser.new({
 "Salute" => [
 [:string],
 ["^Hello$|^Namaste$"]
 ],
 "Name" => [
 [:string],
 []
 ] ,
 "Age" => [
 [:number],
 []
 ] 
 })}
 context "#with valid command" do 
 context "with range" do
 before {
 @ran = false
 @called = command_parser.parse "Salute Hello" do |m,a|
 @ran = true
 @m = m
 @a = a
 end
 }
 it { expect(@ran).to be true }
 it { expect(@called).to be true } 
 it { expect(@a.size).to be 1 } 
 it { expect(@a[0]).to match "Hello" } 
 it { expect(@m).to match "Salute" } 
 end
 context "without range" do 
 context "with string" do 
 before {
 @ran = false
 @called = command_parser.parse "Name Tom" do |m,a|
 @ran = true
 @m = m
 @a = a
 end
 }
 it { expect(@ran).to be true }
 it { expect(@called).to be true }
 it { expect(@a.size).to be 1 } 
 it { expect(@a[0]).to match "Tom" } 
 it { expect(@m).to match "Name" } 
 end
 context "with number" do
 before {
 @ran = false
 @called = command_parser.parse "Age 50" do |m,a|
 @ran = true
 @m = m
 @a = a
 end
 }
 it { expect(@ran).to be true }
 it { expect(@called).to be true }
 it { expect(@a.size).to be 1 } 
 it { expect(@a[0]).to be 50 } 
 it { expect(@m).to match "Age"} 
 end
 end 
 end 
 context "#with invalid command" do 
 before {
 @ran = false
 @called = command_parser.parse "Salute Hi" do |m,a|
 @ran = true
 end 
 }
 it { expect(@called).to be false }
 it { expect(@called).to be false } 
 end
end

direction_spec.rb

require_relative '../spec_helper'
require_relative '../../lib/robot'
include Robot
describe Direction do
 describe ":: finds directions" do
 it { expect( Direction.find "NORTH" ).to be Direction.NORTH }
 it { expect( Direction.find "SOUTH" ).to be Direction.SOUTH }
 it { expect( Direction.find "EAST" ).to be Direction.EAST }
 it { expect( Direction.find "WEST" ).to be Direction.WEST }
 end
 describe ":: turns left" do
 it { expect( Direction.left Direction.NORTH ).to be Direction.WEST }
 it { expect( Direction.left Direction.SOUTH ).to be Direction.EAST }
 it { expect( Direction.left Direction.EAST ).to be Direction.NORTH }
 it { expect( Direction.left Direction.WEST ).to be Direction.SOUTH }
 end
 describe ":: turns right" do
 it { expect( Direction.right Direction.NORTH ).to be Direction.EAST }
 it { expect( Direction.right Direction.SOUTH ).to be Direction.WEST }
 it { expect( Direction.right Direction.EAST ).to be Direction.SOUTH }
 it { expect( Direction.right Direction.WEST ).to be Direction.NORTH }
 end
end

position_spec.rb

require_relative '../spec_helper'
require_relative '../../lib/robot'
include Robot
describe Position do
 context "#moves correctly to north" do
 before {
 @position = Position.new(2, 3, Direction.NORTH)
 @curr_position = @position.move
 }
 it { expect(@curr_position.x).to be 2 }
 it { expect(@curr_position.y).to be 4 }
 it { expect(@curr_position.direction).to be Direction.NORTH }
 end
 context "#moves correctly to south" do
 before {
 @position = Position.new(2, 3, Direction.SOUTH)
 @curr_position = @position.move
 }
 it { expect(@curr_position.x).to be 2 }
 it { expect(@curr_position.y).to be 2 }
 it { expect(@curr_position.direction).to be Direction.SOUTH }
 end
 context "#moves correctly to east" do
 before {
 @position = Position.new(2, 3, Direction.EAST)
 @curr_position = @position.move
 }
 it { expect(@curr_position.x).to be 3 }
 it { expect(@curr_position.y).to be 3 }
 it { expect(@curr_position.direction).to be Direction.EAST }
 end
 context "#moves correctly to west" do
 before {
 @position = Position.new(2, 3, Direction.WEST)
 @curr_position = @position.move
 }
 it { expect(@curr_position.x).to be 1 }
 it { expect(@curr_position.y).to be 3 }
 it { expect(@curr_position.direction).to be Direction.WEST }
 end
 context "#turns left" do
 before {
 start_position = Position.new(2, 3, Direction.WEST)
 @position = start_position.left
 }
 it { expect(@position.x).to be 2 }
 it { expect(@position.y).to be 3 }
 it { expect(@position.direction).to be Direction.SOUTH }
 end
 context "#turns right" do
 before {
 start_position = Position.new(2, 3, Direction.WEST)
 @position = start_position.right
 }
 it { expect(@position.x).to be 2 }
 it { expect(@position.y).to be 3 }
 it { expect(@position.direction).to be Direction.NORTH }
 end
end

simualtor_spec.rb

require_relative '../spec_helper'
require_relative '../../lib/robot'
include Robot
describe Simulator do
 describe "#gets placed" do
 before {
 @simulator = Simulator.new 5,5
 @simulator.place 2, 3, "NORTH"
 }
 it { expect(@simulator.toy_robot.position).to be Position.new(2,3,Direction.NORTH) }
 end
 describe "#moves" do
 context "when inside table" do
 before {
 @simulator = Simulator.new 5,5
 @simulator.place 2, 3, "NORTH"
 @simulator.move
 }
 it { expect(@simulator.toy_robot.position).to be Position.new(2,4,Direction.NORTH) }
 end
 context "when at edge of table" do
 before {
 @simulator = Simulator.new 5,5
 @simulator.place 4, 4, "NORTH"
 @simulator.move
 }
 it { expect(@simulator.toy_robot.position).to be Position.new(4,4,Direction.NORTH) }
 end
 end
 describe "# turns" do
 context "when faced north" do
 before {
 @simulator = Simulator.new 5,5
 @simulator.place 2, 2, "NORTH" 
 }
 it { @simulator.left ; expect(@simulator.toy_robot.position).to be Position.new(2,2,Direction.WEST) }
 it { @simulator.right ; expect(@simulator.toy_robot.position).to be Position.new(2,2,Direction.EAST) }
 end
 context "when faced south" do
 before {
 @simulator = Simulator.new 5,5
 @simulator.place 2, 2, "SOUTH" 
 }
 it { @simulator.left ; expect(@simulator.toy_robot.position).to be Position.new(2,2,Direction.EAST) }
 it { @simulator.right ; expect(@simulator.toy_robot.position).to be Position.new(2,2,Direction.WEST) }
 end 
 context "when faced east" do
 before {
 @simulator = Simulator.new 5,5
 @simulator.place 2, 2, "EAST" 
 }
 it { @simulator.left ; expect(@simulator.toy_robot.position).to be Position.new(2,2,Direction.NORTH) }
 it { @simulator.right ; expect(@simulator.toy_robot.position).to be Position.new(2,2,Direction.SOUTH) }
 end 
 context "when faced west" do
 before {
 @simulator = Simulator.new 5,5
 @simulator.place 2, 2, "WEST" 
 }
 it { @simulator.left ; expect(@simulator.toy_robot.position).to be Position.new(2,2,Direction.SOUTH) }
 it { @simulator.right ; expect(@simulator.toy_robot.position).to be Position.new(2,2,Direction.NORTH) }
 end 
 end
end

table_spec.rb

require_relative '../spec_helper'
require_relative '../../lib/robot'
include Robot
describe Table do
 context "#checks boundry" do
 before { @t = Table.new 5,5 }
 it {expect(@t.inside?(2,3)).to be true}
 it {expect(@t.inside?(0,0)).to be true}
 it {expect(@t.inside?(5,5)).to be false}
 it {expect(@t.inside?(5,3)).to be false}
 it {expect(@t.inside?(3,5)).to be false}
 it {expect(@t.inside?(6,3)).to be false}
 it {expect(@t.inside?(3,6)).to be false}
 it {expect(@t.inside?(-1,0)).to be false}
 it {expect(@t.inside?(0,-1)).to be false}
 end 
end

Answer 1

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
 op, args = cmd.split(/\s+/, 2)
 args = args.split(/\s*,\s*/) if args
 raise InvalidCommand.new unless Commands.public_method_defined?(op)
 begin
 send(op, *args)
 rescue ArgumentError
 raise InvalidCommand.new
 end
 end
 def execute_script(io)
 io.each_line do |line|
 begin
 execute(line)
 rescue InvalidCommand
 #puts "(Ignored invalid command)"
 end
 end
 end
 private
 def validate(x, y, f)
 raise InvalidCommand.new unless x && y && f
 raise InvalidCommand.new unless (0..@board_size) === x
 raise InvalidCommand.new unless (0..@board_size) === y
 end
end

Robot.new.execute_script(ARGF)