Reek codesmell Duplicate Method call fix

Question

I'm getting below errors from reek:

lib/actions.rb -- 5 warnings:
  Actions#move_forward calls (self.x_coordinate + unit) twice (DuplicateMethodCall)
  Actions#move_forward calls place((self.x_coordinate + unit), self.y_coordinate, self.direction) twice (DuplicateMethodCall)
  Actions#move_forward calls self.direction 5 times (DuplicateMethodCall)
  Actions#move_forward calls self.x_coordinate 4 times (DuplicateMethodCall)
  Actions#move_forward calls self.y_coordinate 4 times (DuplicateMethodCall)

Below is the method move_forward

def move_forward(unit = 1)
        case self.direction
        when Direction::SOUTH
            place(self.x_coordinate, self.y_coordinate - unit, self.direction)
        when Direction::EAST
            place(self.x_coordinate + unit, self.y_coordinate, self.direction)
        when Direction::NORTH
            place(self.x_coordinate, self.y_coordinate + unit, self.direction)
        when Direction::WEST
            place(self.x_coordinate - unit, self.y_coordinate, self.direction)
        else

        end
    end

I want to remove all errors especially duplicate method call. What would be the best way to fix all warnings in this case?

Answer 1

Maybe something like this?

def move_forward(unit = 1)
  x, y, d = x_coordinate, y_coordinate, direction
  case d
  when Direction::SOUTH
    place(x, y - unit, d)
  when Direction::EAST, Direction::WEST
    place(x + unit, y, d)
  when Direction::NORTH
    place(x, y + unit, d)
  else
  end
end

I find the complaints about the "duplicate calls" to self.x_coordindate and self.y_coordinate kinda false-positivey though, they're only called once per path.

Answer 2

The code "smell" that is triggering the reek reports is that you are calling to a method that sets state of several instance variables, when a lot less is changing in practice (for instance direction doesn't change at all). The place method sets everything, which makes using it to make a small change overly verbose.

This would probably factor the method down to cause less reported problems:

def move_forward(unit = 1)
  case direction
  when Direction::SOUTH
    move_relative( 0, -unit )
  when Direction::EAST
    move_relative( unit, 0 )
  when Direction::WEST
    move_relative( -unit, 0 )
  when Direction::NORTH
    move_relative( 0, unit )
  end
end

def move_relative( delta_x, delta_y )
  place( x_coordinate + delta_x, y_coordinate + delta_y, direction )
end

(Also I couldn't resist "fixing" your WEST movement, sorry if that's actually wrong)

Answer 3

You don't seem to be using the power of object orientation

What about a solution that instead uses the power of your pre-existing objects?

def move_forward(by_how_much = 1) 
  move_relative(*direction.change_in_coords(by_how_much))
end

def move_relative(delta_x, delta_y)
  place( x_coordinate + delta_x, y_coordinate + delta_y, direction )
end

class Direction::SOUTH
  def self.change_in_coords(unit = 1)
    [0, -unit]
  end
end