2

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

  • 1
    How do you define readable, extensible, and maintainable? – Greg Burghardt Jan 19 at 17:41
  • “Clean code is simple and direct. Clean code reads like well-written prose. Clean code never obscures the designer's intent but rather is full of crisp abstractions and straightforward lines of control. – Anirudh Jan 19 at 17:43
  • 2
    I can't quite see why you are not using the || operator. – gnasher729 Jan 19 at 18:00
  • 2
    @gnasher729 about 40 conditions cluttered into one if statement with || would be even worse and a unit testing nightmare: o – Anirudh Jan 19 at 18:04
2

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.

  • Though I get your point with the short circuit OR now, with lambdas you have the advantage of usability, if you needed to quickly mix and match conditions or reuse the codntiions somewhere else then you wouldn't have duplicates. – Anirudh Jan 20 at 6:17
  • With checker interfacec I would always extend it for other entities as well. Not only the 2 I mentioned. – Anirudh Jan 20 at 6:19
1

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;

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.