Refactor approach for huge if else Clutter based on many independant conditions

Question

Currently I am cleaning up hard to maintain and test if else clutter which is based on conditions which have to be checked in isolations:

What is the basic semantic of the conditions?

Big Entity Objects have to be checked based on two entity keys namely Trans and Right for state change as in the example Below:

if (oldTrans.getfOrder().equals(newTrans.getfOrder()) {
     compound.setIsStateChanged(true);
      return;
}
if (oldRight.getgRight().equals(newRight.getgRight()) {
     compound.setIsStateChanged(true);
}
  ....and the list goes on till 20 more such conditions

Currently the if else are all cluttered up at one place:

    if (oldTrans.getfOrder().equals(newTrans.getfOrder()) {
        compound.setIsStateChanged(true);
        LOGGER.info("major change detected");
        return compound;
    } if (oldTrans.getgOrder().equals(newTrans.getgOrder()) {
        compound.setIsStateChanged(true);
LOGGER.info("major change detected");
        return compound;   
    }

I see 2 main issues here

1. Every if has a return statement and with so many ifs it hard to know when and a what point method exits.

2. To many if branchings are error prone and The number of conditions is likely to go up.

To avoid so many ifs that are basically based on the same semantic underneath from clean code perspective I tried to solve it the polymorphic way

Extracting the Conditions in Enums as Constants and impleneting a checker Interface that takes new and old objects as params

    public interface ICheckStateChange<T>(T old, T new) {
        boolean check(T old, T new);
    }

    //implementations
    public TransChecker implements ICheckStateChange<Trans> {

      List<BiPredicate<Trans, Trans>> allTransConditions = transConditions.getValues();

    public boolean check(Trans oldTrans, Trans newTrans) {
         //all conditions null check here
        //loop through conditions
        for (BiPredicate<Trans, Trans> transCondition: allTransConditions) {
            if (transCondition).test()) {
                return true;
             LOGGER.info("major state change detected, taking apt action")
          }
    }

public RightChecker implements ICheckStateChange<Right> {

      List<BiPredicate<Right, Right>> allTransConditions = RightConditions.getValues();

    public boolean check(Right oldRight, Right newRIght) {
         //all conditions null check here
        //loop through conditions
        for (BiPredicate<Right, Right> rightCondition: allRightConditions) {
            if (rightCondition).test()) {
                return true;
             LOGGER.info("major state change detected, taking apt action")
          }
    }

The Conditons are now centrally located as BiPredicate constants using lambdas

public enum rightConditions {
    FORDER_CHANGE_NULL_TO_NOT_NULL((Order old, Order new)
       -> old == null && new != null),

    //to be replaced by the right condition
    GORDER_CHANGE_FROM_OPEN_TO_DONE((Order old, Order new)
       -> old == null && new != null)

    //to be replaced by the right condition
    LORDER_CHANGE_FROM_OPEN_TO_REVERTED((Order old, Order new)
       -> old == null && new != null)
   }

My question here is about the approach of refactoring the If elses with the help of lambda BiPredicates in hindsight of clean code? Readability, extensibility and maintainability ;)

Answer 1

Let's discuss options. The option you seem to be dismissing is the one that is actually really clean:

public void resolveChanges() {
    boolean isChanged = false;
    isChanged = isChanged || oldTrans.getfOrder().equals(newTrans.getfOrder());
    isChanged = isChanged || oldRight.getfRight().equals(newRight.getfRight());
    // etc.

    compound.setIsStateChanged(isChanged);
}

This is no more a maintenance nightmare or unit testing nightmare than what you had before. All the conditions are still there that were there before. It is however, imminently more readable. The Boolean short cutting is still in effect.

NOTE: I copied and pasted your code here, but saying that one thing is equal to another and claiming a change seems wrong... but I don't know your code base.

---

In line with your refactor, would be using lambdas. Essentially you would set up all the lambdas in an array, and iterate through them. The problem with this is there is a bit of set up that is not necessarily collocated with the rest of the code, so following it is going to be a bit more difficult.

public interface Predicate {
    boolean test();
}

public class TheBigClass {
    private final static Predicate[] resolvers = new Predicate[] {
        () -> oldTrans.getfOrder().equals(newTrans.getfOrder()),
        () -> oldRight.getfRight().equals(newRight.getfRight())
    }

    public void resolveChanges() {
        for (Predicate p : resolvers) {
            if (p.test()) {
                compound.setIsStateChanged(true);
                return;
            }
        }
    }
}

Or some such variation like that. Again, the individual tests are about the same, and the complexity of writing unit tests doesn't change at all from the initial code.

Answer 2

If you like detailed line information in your exceptions:

bool changeDetected = false;

changeDetected = changeDetected || oldTrans.getfOrder().equals(newTrans.getfOrder());
changeDetected = changeDetected || oldTrans.getgOrder().equals(newTrans.getgOrder());

if (changeDetected)
{
    compound.setIsStateChanged(true);
    LOGGER.info("major change detected");
}
return compound;

Otherwise if you prefer less typing:

if (   oldTrans.getfOrder().equals(newTrans.getfOrder())
   ||  oldTrans.getgOrder().equals(newTrans.getgOrder())
{
    compound.setIsStateChanged(true);
    LOGGER.info("major change detected");
}
return compound;

If those checks are really annoying you, and you need a pretty name to keep it straight....

bool hasfOrderChanged(Object oldTrans, Object netTrans)
{
    return oldTrans.getfOrder().equals(newTrans.getfOrder())
}
bool hasgOrderChanged(Object oldTrans, Object netTrans)
{
    return oldTrans.getgOrder().equals(newTrans.getgOrder())
}

...snip...
if (   hasfOrderChanged(oldTrans, newTrans)
   ||  hasgOrderChanged(oldTrans, newTrans)
{
    compound.setIsStateChanged(true);
    LOGGER.info("major change detected");
}
return compound;