Class to verify if a rectification is upgradable

Question

Today I encountered something that I have been wondering about in the past a few times before. How can I refactor a method that has this format:

private boolean isRectificationUpgradable(Rectification rectification) {

    final boolean validType = rectification.getType() == RectificationType.VH
            || rectification.getType() == RectificationType.COL;

    if (!validType) {
        return false;
    }

    if (rectification.getStatus() != RectificationStatus.IN_PROGRESS && rectification.getStatus() != RectificationStatus.OPEN) {
        return false;
    }

    if (rectification.getVehicle() == null) {
        return false;
    }

    final List<CarPassCertificate> certificates = rectification.getVehicle().getCertificates();

    if (CollectionUtils.isEmpty(certificates)) {
        return false;
    }

    CarPassCertificate validCertificate = getValidCertificate(rectification);

    if (validCertificate == null) {
        return false;
    }

    return true;
}

When a rectification is created it has the status "open". An employee indicates that he started to handle it by putting it in the status "in progress".

The upgradable in this context means that sometimes the creator chose a certain type for the rectification, when in fact he could have picked a more accurate other type. In this case someone in the application can "upgrade" this rectification to the more specific type.

In code, this "upgraded" type is not modeled as another class as it is not really an upgrade, it always stays the same Rectification class and some shared additional fields are set to make it a bit more specific.

I think it is pretty readable but as more validations are added it can become very long. Any ideas?

Answer 1

For chained, cascading conditions like you have, there's really nothing wrong with what you are doing (conceptually). A sequence of if-statements identifying invalid conditions, and returning false if invalid, is just fine.

A good idea is to always organize the most common reason to be invalid first, so that you reject values with the least amount of effort overall.

Finally, it makes little sense, other than for debugging purposes, to have temporary variables to hold state. So, for example, the following:

final boolean validType = rectification.getType() == RectificationType.VH
        || rectification.getType() == RectificationType.COL;

if (!validType) {
    return false;
}

should just be:

if (rectification.getType() != RectificationType.VH
        && rectification.getType() != RectificationType.COL) {
    return false;
}

h.j.k already pointed out that the last boolean statement can often be implemented as a single return, but even that is something that I feel is optional since the pattern of conditions is often more important to keep consistent, than the last condition being "small". Consistent conditions allow for easier maintenance of the code too (adding a condition is a simple add, not a modification of an existing short return).

Putting these suggestions together I would happily "Pass" the following code in a review:

private boolean isRectificationUpgradable(Rectification rectification) {

    if (rectification.getType() != RectificationType.VH
            && rectification.getType() != RectificationType.COL) {
        return false;
    }

    if (rectification.getStatus() != RectificationStatus.IN_PROGRESS 
            && rectification.getStatus() != RectificationStatus.OPEN) {
        return false;
    }

    if (rectification.getVehicle() == null) {
        return false;
    }

    if (CollectionUtils.isEmpty(rectification.getVehicle().getCertificates())) {
        return false;
    }

    if (getValidCertificate(rectification) == null) {
        return false;
    }

    return true;
}

Answer 2

This looks as if it is part of the upgrade code itself. I think you have too low a level of abstraction in here and you are also mixing different concerns. Also, your workflow just seems fragile.

Cars and certificates

You check that there is a car, then you check that it has any certificates, then you pass the rectification to getValidCertificate. Either you only need to pass the certificate list or you don't need to do the "has car/certificates" check yourself, since getValidCertificates clearly has to do that itself to be able to return a CarPassCertificate (or null).

(Since starting this question, we've discussed this issue in comments and I see you've agreed that only getValidCertificates needs to do the check. Nice.)

Workflow

How can a rectification be both open and in progress and yet not even have a car? Even if it is legitimate to have one open with no car, I can't see how it is valid for an in-progress task. I really think your type hierarchy may not properly model your workflow.

If a rectification may exist for a period without a car (let's think of it as a rectification request) but

1. Rectifications can only be actioned once a car is added
2. Cars are not removed from Rectifications once added

then you should have two different classes to represent them. The first type should not have a car field at all. The second should not be creatable without being passed a non-null car object. Depending on the rest of your code, the second class could be a subclass of the first (with the addition of a car field amongst other things) or share an interface with the first (preferred option of those two) or the request could be an entirely separate class, replaced with a car-containing rectification by a factory.

Either way, isRectificationUpgradable only need specify the second type as input. And immediately a whole category of errors is eliminated. There is no need to check for the existence of a car when it is guaranteed to be present.

If you do it this way, you never have to check for the presence of a real car. Any method that depends on the existence of a car simply has to specify the car-owning type.

I would not be surprised if other stages in the lifecycle of rectifications can be treated this way. If you create a common rectification interface but

• only the OpenRectification class/interface (which has no employee field) has a toAssigned method which requires an employee object and returns an assignedRectification object
• only the assignedRectification type has changeAssignedEmployee method and toInProgress methods
• only thetoInProgress type has methods which can be used to (for example) log work done

then many more categories of error evaporate.

Answer 3

1. Standardize your comparisons

   You have two similar comparisons here (I'm assuming they are enum - or gasp, primitive - values for == to work correctly):

   !(rectification.getType() == RectificationType.VH
       || rectification.getType() == RectificationType.COL)
   
   rectification.getStatus() != RectificationStatus.IN_PROGRESS
       && rectification.getStatus() != RectificationStatus.OPEN
   

   You should choose whether you want to 'describe' such comparisons as 'not either the following', or 'not this and not that'. When you stick to one description, you don't have to context-switch between the various logical operators... just one common understanding will do. :) For illustration, here is the same comparison with the latter:

   if (rectification.getType() != RectificationType.VH
           && rectification.getType() != RectificationType.COL) {
       return false;
   }
   if (rectification.getStatus() != RectificationStatus.IN_PROGRESS 
           && rectification.getStatus() != RectificationStatus.OPEN) {
       return false;
   }
   

   If you find doing multiple logical operators is getting out of hand, you can also use a Collection to help you out (once again, assuming enum values here, else something like a Set<Integer>):

   // skipping 'Rectification*.' on the values for brevity
   Set<RectificationType> validType = EnumSet.of(VH, COL);
   Set<RectificationStatus> validStatus = EnumSet.of(IN_PROGRESS, OPEN);
   if (!validType.contains(rectification.getType()) &&
           !validStatus.contains(rectification.getStatus()) {
       return false;
   }
   

2. Group comparisons together

   Next, you check whether rectification.getVehicle() is non-null, and if so whether its getCertificates() returns empty. Therefore, you may want to consider grouping this into a single if-statement:

   if (rectification.getVehicle() == null || 
       CollectionUtils.isEmpty(rectification.getVehicle().getCertificates())) {
       return false;
   }
   

3. Consolidate final if-else

   Usually, if (pun unintended) there is a final if statement with a return statement, followed by a final return, one can easily consolidate that as one return statement using the ternary operator. In your case, this results in:

   return getValidCertificate(rectification) != null;
   

Answer 4

I suggest you to delegate the responsibility for each rule to evaluate itself. Each rule is implemented in a separate class.

public interface IRule<T> {
    boolean test(T value);
}

public final class RectificationTypeRule implements IRule<Rectification> {
    @Override
    public final boolean test(Rectification r) {
        //TODO: test on r.getType() as you need
    }
}

public final class RectificationStatusRule implements IRule<Rectification> {
    @Override
    public final boolean test(Rectification r) {
        //TODO: test on r.getStatus() as you need
    }
}

...

Your method could then use a collection of rules (theses rules can be provided by a factory for example) and test against each to see if a Rectification is valid (or upgradable in your case).

private boolean isRectificationUpgradable(Rectification rectification) {
    for(IRule<Rectification> rule : rules) {
        if(!rule.test(rectification)) {
            return false;
    }
    return true;
}

You can add as much rules as you want in the future and the readability of the method won't be impacted.

Plus it allows you to switch from a ruleSet to another at runtime (make your program more evolutive).