refactoring Control Parameter

Question

I'm using a tool for finding code smells in code called reek and I have a problem with one called Control Parameter

def place_ship(ship, start_position, orientation)
    @row = start_position[:row]
    @column = start_position[:column]
    ship.length.times do
        if orientation == :vertical
            vertical_place_ship(row,column,ship)
        else
            horizontal_place_ship(row,column,ship)
        end
    end
end

def vertical_place_ship(row,column,ship)
    self.grid[row][column].ship = ship
    self.grid[row][column].status = :occupied
    @row += 1 
end

def horizontal_place_ship(row,column,ship)
    self.grid[row][column].ship = ship
    self.grid[row][column].status = :occupied
    @column += 1
end

warning's content: [

55]:ControlParameter: Board#place_ship is controlled by argument 'orientation

How do I fix this?

If you look at the docs for Reek here http://www.rubydoc.info/github/troessner/reek/Reek/Smells/ControlParameter it actually mentions the Strategy Pattern as one solution. I don't know Ruby well enough to do a code example, but hopefully someone else can. — Kerri Brown, Jan 07 '18 at 09:16

Kavitha · Answer 1 · 2018-01-07T16:54:14.257

'Orientation' is a flag value in place_ship method. Value of 'orientation' is not changing as the code executes. So no need to check it 'ship.length' times.

place_ship has only conditional logic and nothing else. This is unnecessary and the conditional logic can reside outside. You are passing in a flag, that tells the method what path to choose, conditionally. This is the Conditional Coupling smell. Generally do not pass in a conditional parameter to a method. Have 2 different methods for the 2 choices and name them aptly.

You already have aptly named vertical_place_ship and horizontal_place_ship methods. You can refactor it like this.

def <method_that_calls_place_ship>
// other code
    if orientation == :vertical
      vertical_place_ship(ship, start_position)
    else
      horizontal_place_ship(ship, start_position)
    end
// more code
end

def vertical_place_ship(ship, start_position)
    row = start_position[:row]
    column = start_position[:column]

    ship.length.times do
      self.grid[row][column].ship = ship
      self.grid[row][column].status = :occupied
      row += 1 
    end  
end

Similarly for horizontal_place_ship method.

score 0 · Answer 2 · answered Jan 07 '18 at 15:38

0

Regardless of the tool feedback, looking at your code, the only line that differ between the horizontal & vertical case is whether to increase @rows or @columns. An option could be:

def place_ship(ship, start_position, orientation)
    row = start_position[:row]
    column = start_position[:column]
    ship.length.times do
        self.grid[row][column].ship = ship
        self.grid[row][column].status = :occupied
        orientation == :vertical ? row += 1 : column += 1
    end
end

I removed the two (identical) methods, and just used the ternary operator ('?') to increase the correct variable after placing each ship part.

answered Jan 07 '18 at 15:38

Martin

7,634
1
20
23

Orientation is being checked multiple times within the loop. But it's value never changes. – Kavitha Jan 07 '18 at 16:55
This still has the same code smell as the original code. You have only changed the structure of the conditional statement. – Kerri Brown Jan 07 '18 at 17:35

refactoring Control Parameter

2 Answers2