10
\$\begingroup\$

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?

\$\endgroup\$
8
  • \$\begingroup\$ Are you on Java 8? \$\endgroup\$
    – h.j.k.
    Sep 15 '15 at 10:03
  • \$\begingroup\$ No, on java 7 at the moment. \$\endgroup\$
    – geoffreydv
    Sep 15 '15 at 10:05
  • 1
    \$\begingroup\$ How can a rectification be open and in progress and not have a car? \$\endgroup\$
    – itsbruce
    Sep 15 '15 at 11:59
  • 1
    \$\begingroup\$ But since getValidCertificate returns CarPassCertificate, it must itself be fetching the car's certificates. You're duplicating code here, as far as I can see. You're doing a couple of checks which getValidCertificate will do for you. \$\endgroup\$
    – itsbruce
    Sep 15 '15 at 13:17
  • 1
    \$\begingroup\$ @itsbruce, ok, now I understand. What you are saying is correct, I have moved that specific check to the getValidCertificate method, which is a better place. That cleans up the main method. Thank you. \$\endgroup\$
    – geoffreydv
    Sep 15 '15 at 13:36
5
\$\begingroup\$

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;
}
\$\endgroup\$
1
  • \$\begingroup\$ Thanks for pointing out that the variables should not be used. I first thought that it would give more "meaning" to what that if is checking, but in this specific case it doesn't really do that and just clutters the method. After adding a few suggestions from other answers I decided that the code is clear enough right now (like you suggested). I'm accepting yours as the best answer. \$\endgroup\$
    – geoffreydv
    Sep 15 '15 at 13:26
7
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you so much for this answer @itsbruce. I really like how you went beyond what I was asking and question the whole workflow itself. I never thought about it this way before and it seems I am not leveraging the power of object oriented design enough. While your points are absolutely right, it is too late in this project to make such fundamental changes but you got the wheels in my head cranking and I'll be sure to think more in this way in the next one. \$\endgroup\$
    – geoffreydv
    Sep 16 '15 at 7:21
5
\$\begingroup\$
  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;
    
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Regarding point 3, this looks like code into which successive validation rules have been added and which may receive more. If further validation rules are required but are only worth consideration where the car is valid, then the OP is probably better off leaving that final return alone. Although if that is true, this code really needs to be better organised. \$\endgroup\$
    – itsbruce
    Sep 15 '15 at 11:27
  • 1
    \$\begingroup\$ @h.j.k, thanks for pointing out the differences between the boolean operators, I chose to change them to a common understanding like you suggested and introduced a method on the enum. (I'll update my question in a few minutes). I also grouped the comparisons together. As for the final if else, I left that intact as it looks more readable to me that way. \$\endgroup\$
    – geoffreydv
    Sep 15 '15 at 13:22
  • \$\begingroup\$ @geoffreydv glad to help. :) \$\endgroup\$
    – h.j.k.
    Sep 15 '15 at 14:03
4
\$\begingroup\$

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

\$\endgroup\$
4
  • 1
    \$\begingroup\$ The user is not on Java 8, so the Predicate is not available. Still this is a reasonable review. I would suggest though, that you use the functional-style of predicate creation, though. \$\endgroup\$
    – rolfl
    Sep 15 '15 at 12:56
  • \$\begingroup\$ @rolfl I've added a custom interface that mimic the Java8 Predicate. Could you detail what you mean by "functional-style of predicate creation" please ? \$\endgroup\$
    – Spotted
    Sep 15 '15 at 13:02
  • \$\begingroup\$ I think this is a great answer but for my specific case I think it is too complicated. It would take someone more than just a quick glance at the method to understand what is going on. When there are a bunch more rules I would choose this method, thanks! \$\endgroup\$
    – geoffreydv
    Sep 15 '15 at 13:16
  • \$\begingroup\$ @geoffreydv does isRectificationUpgradable form part of validation in the sense of object validation (that is to say, an upgrade-type object will not be created if this check fails) or is it validation in the sense of enforcing business rules? If the former, the solution here probably is overkill. If the latter, it's a very good solution. \$\endgroup\$
    – itsbruce
    Sep 15 '15 at 14:43

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.