Are there any techniques to split a method with a flag argument?

Question

I have a method with a flag argument. I think that passing a boolean to a method is a bad practice (complicates the signature, violates the "each method does one thing" principle). I think splitting the method into two different methods is better. But if I do that, the two methods would be very similar (code duplication).

I wonder if there are some general techniques for splitting methods with a flag argument into two separate methods.

Here's the code of my method (Java):

int calculateNumOfLiveOrDeadNeighbors(Cell c, int gen, boolean countLiveOnes) {
   int x = c.getX();
   int y = c.getY();
   CellState state;
   int aliveCounter = 0;
   int deadCounter = 0;
   for (int i = x - 1; i <= x + 1; i++) {
      for (int j = y - 1; j <= y + 1; j++) {
         if (i == x && j == y)
            continue;
         state = getCell(i, j).getCellState(gen);
         if (state == CellState.LIVE || state == CellState.SICK){
            aliveCounter++;
         }
         if(state == CellState.DEAD || state == CellState.DEAD4GOOD){
            deadCounter++;
         }
      }
   }
   if(countLiveOnes){
      return aliveCounter;
   }
   return deadCounter;
}

Wellinformed

Answer 1

I guess it depends on every single case.

In this example you have two choices, in my opinion.

Say you want to split the call calculateNumOfLiveOrDeadNeighbors()

in two:

calculateNumOfLiveNeighbors() 

and

calculateNumOfDeadNeighbors()

You can use Template Method to move the loop to another method. You can use it to count dead / alive cells in the two methods.

private int countCells(Cell c, int gen, Filter filter)
{
    int x = c.getX();
    int y = c.getY();
    CellState state;
    int counter = 0;
    for (int i = x - 1; i <= x + 1; i++) 
    {
        for (int j = y - 1; j <= y + 1; j++) 
        {
            if (i == x && j == y)
                continue;
            state = getCell(i, j).getCellState(gen);
            if (filter.countMeIn(state))
            {
                counter++;
            }
        }
    }
    return counter;
 }

 private interface Filter
 {
      boolean countMeIn(State state);
 }

 public int calculateNumOfDeadNeighbors(Cell c, int gen)
 {
     return countCells(c, gen, new Filter()
                       { 
                           public boolean countMeIn(CellState  state)
                           {
                              return (state == CellState.DEAD || state == CellState.DEAD4GOOD);
                           }
                        });
  }

 public int calculateNumOfLiveNeighbors(Cell c, int gen)
 {
     return countCells(c, gen, new Filter()
                       { 
                           public boolean countMeIn(CellState  state)
                           {
                              return (state == CellState.LIVE || state == CellState.SICK);
                           }
                        });
  }

It's cumbersome, maybe not even worth the pain. You can, alternatively, use a monad to store the results of your statistics calculation and then use getDeadCounter() or getLiveCounter() on the monad, as many suggested already.

Wallachia

Good idea, but use static inner classes to implement the Filter. – Immaculate

Answer 2

If you don't like the boolean on your signature, you could add two different methods without it, refactoring to private the main one:

int calculateNumOfLiveNeighbors(Cell c, int gen) {
  return calculateNumOfLiveOrDeadNeighbors(c, gen, true);
}
int calculateNumOfDeadNeighbors(Cell c, int gen) {
  return calculateNumOfLiveOrDeadNeighbors(c, gen, false);
}

OR

you could code a Result Class or int array as output parameter for storing both the results; this would let you get rid of the annoying boolean parameter.

Lagerkvist

I'd prefer the private method with public delegate-methods over anything else suggested. – Nonpartisan

Answer 3

• you can try to extract the common functionality in a single method and only use the specific functionality
• you can create a private method with that flag, and invoke it from the two public methods. Thus your public API will not have the 'complicated' method signature, and you won't have duplicated code
• make a method that returns both values, and choose one in each caller (public method).

In the example above I think the 2nd and 3rd options are more applicable.

Scabby

Answer 4

Seems like the most semantically clean approach would be to return a result object that contains both values, and let the calling code extract what it cares about from the result object.

Monkery

+1 for optimization. Otherwise the method should be called twice to get dead and alive neighbors. – Virtu

Answer 5

IMO, this so-called "each method does one thing" principle needs to be applied selectively. Your example is one where, it is probably better NOT to apply it. Rather, I'd just simplify the method implementation a bit:

int countNeighbors(Cell c, int gen, boolean countLive) {
   int x = c.getX();
   int y = c.getY();
   int counter = 0;
   for (int i = x - 1; i <= x + 1; i++) {
      for (int j = y - 1; j <= y + 1; j++) {
         if (i == x && j == y)
            continue;
         CellState s = getCell(i, j).getCellState(gen);
         if ((countLive && (s == CellState.LIVE || s == CellState.SICK)) ||
             (!countLive && (s == CellState.DEAD || s == CellState.DEAD4GOOD))) {
            counter++;
         }
      }
   }
   return counter;
}

Trolly

Answer 6

Like Bozho said: But but combine point 2 and 3 in the other way arround:

Create a (possible private method) that returns both (living and dead) and (only if you need dead or alive seperate in the most cases) then add two methods that pick dead or both out of the result:

DeadLiveCounter calcLiveAndDead(..) {}
int calcLive(..) { return calcLiveAndDead(..).getLive; }
int calcDead(..) { return calcLiveAndDead(..).getDead; }

Immaculate

Answer 7

In terms of using refactoring, some things you can do are;

• copy the method and create two version, one with true hard coded and the other false hard coded. Your refactoring tools should help you inline this constant and remove code as required.
• recreate the method which calls the right true/false method as above for backward compatibility. You can then inline this method.

Benignant

Answer 8

I would be inclined here to keep a map from the CellState enum to count, then add the LIVE and the SICK or the DEAD and the DEAD4GOOD as needed.

int calculateNumOfLiveOrDeadNeighbors(Cell c, int gen, boolean countLiveOnes) {
    final int x = c.getX();
    final int y = c.getY();
    final HashMap<CellState, Integer> counts = new HashMap<CellState, Integer>();
    for (CellState state : CellState.values())
        counts.put(state, 0);

    for (int i = x - 1; i < x + 2; i++) {
        for (int j = y - 1; j < y + 2; j++) {
            if (i == x && j == y)
                continue;
            CellState state = getCell(i, j).getCellState(gen);
            counts.put(state, counts.get(state) + 1);
        }
    }
    if (countLiveOnes)
        return counts.get(CellState.LIVE) + counts.get(CellState.SICK);
    else
        return counts.get(CellState.DEAD) + counts.get(CellState.DEAD4GOOD);
}

Hookup

Answer 9

have a private method which is an exact copy and paste of what you currently have. Then create two new methods, each with a more descriptive name that simply call your private method with appropriate boolean

Prettypretty