2
\$\begingroup\$

could you please review the code for the implementation of the class (link: General review of Robot class)

require_relative 'robot'
class Game

  def initialize
    # initialize values
    @commands = %w{PLACE MOVE LEFT RIGHT REPORT}
    @facings = %w{NORTH SOUTH EAST WEST}
    @pos_X, @pos_Y = nil, nil
    @facing, @robot, @r = nil, nil, nil
    command = get_initial_command
    setup(command)
  end

  def setup(command)
    params = command.gsub(/[PLACE]/, '').split(',')
    if (params[0] != nil && params[1] != nil && params[2] != nil)
      @pos_X  = params[0].to_i
      @pos_Y  = params[1].to_i
      @facing = params[2]
      @r = Robot.new(@pos_X, @pos_Y, @facing)
      if is_valid_position
        @robot = @r 
      else
        Game.new
      end
      command = get_commands
    else
      Game.new
    end
  end

  def get_initial_command
    command = gets.strip
    if command.include?("PLACE")
      return command
    else  
      command = get_initial_command
    end
  end

  def get_commands
    is_command = false
    command = gets.strip
    @commands.each do |c|
      if command.include?(c)
        is_command = true
        break
      end
    end
    if is_command
      translate_command(command)
      command = get_commands
      return command
    else
      command = get_commands
    end
  end

  def translate_command(command)
    case command
    when command.include?('PLACE')
      puts 'place the command'
      setup(command)
    when 'MOVE'
      @robot.move if is_valid_move
    when 'RIGHT'
      @robot.right
    when 'LEFT'
      @robot.left
    when 'REPORT'
      @robot.report 
    when 'EXIT'
      Kernel.abort(false)
    end
  end

  def is_valid_move
    if ((@robot.instance_variable_get("@pos_X") == 0 && @robot.instance_variable_get("@facing") == 'SOUTH') ||
        (@robot.instance_variable_get("@pos_X") == 5 && @robot.instance_variable_get("@facing") == 'NORTH') ||
        (@robot.instance_variable_get("@pos_Y") == 0 && @robot.instance_variable_get("@facing") == 'WEST') ||
        (@robot.instance_variable_get("@pos_Y") == 5 && @robot.instance_variable_get("@facing") == 'EAST'))
          return false
    else
      return true
    end
  end

  def is_valid_position
    is_proper_facing = false
    @facings.each do |f|
      if f.include?(@r.instance_variable_get("@facing"))
        is_proper_facing = true
        break
      end
    end
    if (@r.instance_variable_get("@pos_X") < 0 || @r.instance_variable_get("@pos_X") > 5 ||
      @r.instance_variable_get("@pos_Y") < 0 || @r.instance_variable_get("@pos_Y") > 5 || !is_proper_facing)
      return false
    else
      return true
    end
  end
end

a = Game.new
\$\endgroup\$
2
\$\begingroup\$

A couple of issues I found or suggestions I have:

  • avoid unnecessary or redundant comments (like # initialize values within initialize)
  • use symbols for your commands and facings
  • don't use upper case letters in your variable names (pos_x or just x instead of pos_X)
  • no need to check for != nil, you can simply do params[0] && params[1] &&params[2]
  • I'd convert simple ifs to use the ternary operator instead: is_valid_position ? @robot = @r : Game.new
  • find a better name for @r
  • I'm not sure why you're using instance_variable_get instead of just @robot.pos_X

Try to refactor your longer methods. Take setup as example, personally, I'd like it to look somewhat like this:

def setup(command)
  begin
    params = get_params(command)
  rescue ParamsInvalidError
    restart
  end

  new_robot(params)
  restart unless is_valid_position?
end
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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