0

I have an old code that needs to be brought back to life, it utilises around 10-15 booleans that dance around the entire class, like so:

if (condition)
{
  bool1 = true
}
if (condition)
{
  bool2 = true
}
...

then

if (bool1 == true && bool2 == true && bool3 == false)
{
  do something
}
else if (bool1 == true && bool2 == false && bool3 == false)
{
  do something
}
...

Could coding this way be avoided? Are there better ways to implement this? Perhaps utilising a map?

I would like to improve readability and overall performance, since this piece of code is over 1,000s lines long.

After feedback adding more specific example:

boolean bool1 = false, bool2 = false, bool3 = false, bool4 = false, bool5 = false,
bool6 = false, bool7 = false, bool8 = false, bool9 = false, bool10 = false;

if (string_object.startsWith("Pattern1"))
{
    bool1 = true
}
if (string_object.startsWith("Pattern2")
{
    bool2 = true
}
if (string_object.startsWith("Pattern3")
{
    bool3 = true
}
if (string_object.startsWith("Pattern4")
{
    bool4 = true
}
if (string_object.startsWith("Pattern5")
{
    bool5 = true
}
// and so on...

if (system_type.equals("type1"))
{
    if (bool1 == true && bool2 == true && bool3 == false)
    {
        system_value.set("value1")
    }
    else if (bool1 == true && bool2 == false && bool3 == false)
    {
        system_value.set("value2")
    }
    else if (bool1 == true && bool3 == false && bool4 == true)
    {
        system_value.set("value3")
    }
}
else if (system_type.equals("type2"))
{
    if (bool1 == true && bool2 == true && bool4 == true)
    {
        system_value.set("value1")
    }
    else if (bool1 == true && bool3 == false && bool5 == true)
    {
        system_value.set("value4")
    }
    else if (bool1 == true && bool3 == false && bool4 == true)
    {
        system_value.set("value5")
    }
}
// and so on...
k4s
  • 149
  • 1
  • 14
  • 8
    1. `bool1 == true` --> `bool1` 2. compression depends on set of conditions. – jmj Oct 12 '17 at 18:47
  • 4
    Depends what the purpose of all the booleans is. Remember that writing code is just as much about communicating with future developers/maintainers as it is about telling the computer what to do. If these booleans have clear meanings in the context of what the program has to do, then just change their names to make those meanings clear. If your code would be more meaningful after a refactoring, then do the refactoring. – Dawood ibn Kareem Oct 12 '17 at 18:48
  • 2
    I would suggest using methods with good names, so you see from the name what these condition actually means, to evaluate your "conditions" and use them instead these `bool1` `bool2`... These way you can incapsulate this logic, avoid using global vars and make code more readable – Sergei Sirik Oct 12 '17 at 18:52
  • Group the booleans into meaningful sets, and encapsulate them in enums. Those can serve as strategy objects containing the executable code in the if statements. I'd post sample code but I'm on mobile. – daniu Oct 12 '17 at 19:22
  • It seems to me that he question is a bit too broad as it currently stands, as the code is too generic and probably overlooks specific cases. On the other hand, if it was actual code, it might be a better fit for [codereview.se]. – Didier L Oct 13 '17 at 09:33
  • I've made the code less generic. – k4s Oct 14 '17 at 13:50

5 Answers5

5

There are a few things you can do.

  1. as others have mentioned, code like this:

    if (condition) { bool1 = true; }

Can be compressed to:

bool1 = (condition);
  1. Another useful tool is one of Martin Fowler's refactors - Decompose Conditional.

An example of this would be something like changing this:

if (bool1 == true && bool2 == true && bool3 == false)
{
  do something
}
else if (bool1 == true && bool2 == false && bool3 == false)
{
  do something
}

To something like this:

if (firstCondition())
{
  do something
}
else if (secondCondition())
{
  do something
}

private boolean firstCondition() {
  return (bool1 && bool2 && !bool3 );
}

private boolean secondCondition() {
  return (bool1 && !bool2 && !bool3);
}

Decomposing complex conditionals like this makes your code easier to read and maintain.

Michael Peacock
  • 2,011
  • 1
  • 11
  • 14
1

You can construct bit maps from your booleans, and encode desired combinations as integers.

Here is an example: let's say you need three booleans, flag0, flag1, and flag2, and you need to check five different combinations of the flags:

flag2 flag1 flag0   Action
----- ----- ----- ----------
 true false false ActionOne
 true  true false ActionTwo
 true false  true ActionThree
false false  true ActionFour
false  true  true ActionFive

Then you can build flags as follows:

int flags = 0;
if (condition0) flags |= (1 << 0);
if (condition1) flags |= (1 << 1);
if (condition2) flags |= (1 << 2);

Now each combination of conditions is encoded as a unique number between zero and seven, inclusive, so it could be checked with a switch statement:

switch(flags) {
    case 4: actionOne();   break; // 1 0 0
    case 6: actionTwo();   break; // 1 1 0
    case 5: actionThree(); break; // 1 0 1
    case 1: actionFour();  break; // 0 0 1
    case 3: actionFive();  break; // 0 1 1
}
Sergey Kalinichenko
  • 714,442
  • 84
  • 1,110
  • 1,523
  • 1
    And even better if you use an Enum to name them – Cosine Oct 12 '17 at 18:57
  • 1
    @Cosine This depends a good deal on the project, but either an enum or a detailed comment is definitely in order here. – Sergey Kalinichenko Oct 12 '17 at 18:58
  • 2
    `int actionOneCase=0b001, actionTwoCase=0b010, actionThreeCase=0b...` or you could use an actual enum object, up to you. As dasblinkenlight (nice name!) says, a thorough comment would also work. – Cosine Oct 12 '17 at 19:02
  • @dasblinkenlight I've added a more detailed code example to my question, it would be interesting to see your answer adjusted. – k4s Oct 15 '17 at 18:15
1

Most of the time this antipattern is due to developers not wanting to create a subclass for just one new behavior. If that is the case, polymorphism could help.

Assume you have the following class:

public class Animal {
  private final boolean isCat;
  private final boolean isReptile;
  private final boolean isDog;


  private Animal(final boolean isCat, final boolean isReptile, final boolean isDog) {
    this.isCat = isCat;
    this.isReptile = isReptile;
    this.isDog = isDog;
  }

  public static Animal getLizard() {
    return new Animal(false, true, true);
  }

  public static Animal getDog() {
    return new Animal(false, false, false);
  }

  public String seeStranger() {
    final StringBuilder result = new StringBuilder(this.toString());
    if (isDog) {
      result.append(" barks and");
    } else if (isCat) {
      result.append(" meows and");
    }
    if (isReptile) {
      result.append(" crawls away.");
    } else {
      result.append(" walks forward.");
    }
    return result.toString();
  }
}

What you really want are multiple classes with differing behavior:

public abstract class Animal {


  public static Animal getLizard() {
    return new Lizard();
  }

  public static Animal getDog() {
    return new Dog();
  }

  public abstract String seeStranger();

  private static class Lizard extends Animal {
    @Override
    public String seeStranger() {
      return this.toString() + " crawls away.";
    }
  }

  private static class Dog extends Animal {
    @Override
    public String seeStranger() {
      return this.toString() + " barks and walks forward.";
    }
  }
}
Andreas
  • 4,937
  • 2
  • 25
  • 35
1

Depending on your use case, you might find it easier to dispense with Boolean variables altogether and just put the conditions inline in the control flow statements. For example, you could change this:

if (condition1)
{
  bool1 = true
}
else if (condition2)
{
  bool2 = true
}

...

if (bool1 == true && bool2 == true && bool3 == false)
{
  do something
}
else if (bool1 == true && bool2 == false && bool3 == false)
{
  do something
}

...

to be something more like this:

if (condition1 && condition2 && condition3)
{
  do something
}
else if (condition1 && (not condition2) && codition3)
{
  do something
}

...

You might also be able to simplify your conditions. For example, there might be a condition4 equivalent to condition1 && condition2 that doesn't use &&. Consider condition1 := x <= 100 and condition2 := x >= 100. condition1 && condition2 is totally equivalent to x == 100, and I would definitely recommend changing

...
bool1 = (x >= 100);
...
bool2 = (x <= 100);
...
if (bool1 == true && bool2 == true) { ... }
...

to this instead:

...
if (x == 100) { ... }
...

I hesitate to go so far as to call the explicit use of Boolean variables an antipattern, but I tend to prefer to keep Booleans as R-values whenever possible.

Patrick87
  • 27,682
  • 3
  • 38
  • 73
0

In this case I would recommend leaving the booleans alone, if they're well labeled

But a neat thing can be done if they are closely related (ie direction, N/S/E/W), called bitmasks Related Stack Overflow post: what is a bitmask and a mask

They're useful for direction if you have a grid of streets, each street intersection can have N/S/E/W roads coming out of it, can be defined as 4 bits of a number

Let's define a few constants

N=1 (0001)
S=2 (0010)
E=4 (0100)
W=8 (1000)

An intersection with roads in N and E would be N|E

N|E=1|4=5 (0101)

A full + intersection (NSEW) would be N|S|E|W

N|S|E|W=1|2|4|8=15 (1111)

If you're adding to a bitmask, newMask = oldMask|direction

Let's add S to our NE mask

int newMask = oldMask | S

oldMask was 0101, S is 0010, newMask becomes 0111

It also has an easy way to check if a direction exists

If we want to check if oldMask contains N

boolean N? = (oldMask & N) != 0

oldMask & N will isolate the N bit, making the returned value either be N or 0

phflack
  • 2,729
  • 1
  • 9
  • 21