Needs code review for class implementation

Question

could you please review the code for the implementation of the class (link: Ruby. Code review. Working alone on the code)

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

Answer 1

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