CODEWITHSUNDEEP
javarefactoringflagscoding-style
snakile
snakile

Reputation: 54521

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

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;
}

Upvotes: 8

Views: 1799

Answers (9)

Carl Manaster
Carl Manaster

Reputation: 40346

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);
}

Upvotes: 1

systempuntoout
systempuntoout

Reputation: 74104

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.

Upvotes: 5

Manrico Corazzi
Manrico Corazzi

Reputation: 11371

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.

Upvotes: 4

hhafez
hhafez

Reputation: 39760

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

Upvotes: 0

Peter Lawrey
Peter Lawrey

Reputation: 533570

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.

Upvotes: 1

Stephen C
Stephen C

Reputation: 718986

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;
}

Upvotes: 1

Ralph
Ralph

Reputation: 120831

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; }

Upvotes: 1

Carl
Carl

Reputation: 27013

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.

Upvotes: 1

Bozho
Bozho

Reputation: 597134

  • 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.

Upvotes: 4

Related Questions