Developing an API to delegate/decouple long Validation Chains

Question

Recently, I was given a task of re-writing a really old piece of software. The whole software itself is well written, except for the one thing that worries me, classes containing a huge amount of code. A lot of that is nothing but really really big validation chains that go like:

if (!isFooValid(dataObject.getFoo()))
    throw new FooException(...);

if (!isBarValid(dataObject.getBar()))
    throw new BarException(...);

And that inspired me to write an API that can help in decoupling the validation chains.

---

As far as my understanding goes, validation is generally a Composite Pattern, and if we break it down and separate the what we want to from the how we want to do it, we get:

If foo is valid then do something.

And we got an abstraction: is valid.

---

So I went up on decoupling how do we get the abstraction is valid, I came up with a design idea.

I can have a Result object, that contains the message about validation with a simple true/false to check whether it was successful or not.

public interface Result {
    // StandardResult is the default implementation of Result.
    // Using the default constructor gives an instance that indicates
    // "SUCCESS" state of validation.
    public static final Result OK = new StandardResult();       
    public Throwable getError();    
    public boolean isOk();    
    public String getMessage();    
}

I can have a Validator<T> object, that has the validation logic, and then returns a Result object that contains information about what happened.

public interface Validator<T> {     
    public Result validate(T target);    
}

This enables me to do stuff like:

Result r = new SomeStringValidator().validate("This String");

And similarly, I can do the validation chains using a Chain of Responsibility pattern.

public class ChainValidator<T> implements Validator<T> {

    // This list contains all the validators that are in the chain.
    private final List<Validator<T>> validators = new ArrayList<Validator<T>>();

    public CompositeResult<T> validate(T target) {
        CompositeResult<T> result = new CompositeResult<T>();
        for (Validator<T> v : validators) {
            Result validationResult = null;
            try {
                validationResult = v.validate(target);
            } catch (Exception ex) {
                // Creating it with StandardResult(Throwable) would give
                // an instance that indicates "FAILED" state.
                validationResult = new StandardResult(ex);
            }
            result.put(v, validationResult);
        }
        return result;
    }

    private final class CompositeResult<T> implements Result {

        private final Map<Validator<T>, Result> delegate = new HashMap<Validator<T>, Result>(); 
        private Throwable failCause;

        public boolean isOk() {
            for (Result r : delegate.values()) {
                if (!r.isOk()) {
                    failCause = r.getError();
                    return false;
                }
            }
            return true;
        }


        public String getMessage() {
            return delegate.toString();
        }

        public void put(Validator<T> validator, Result resut) {
            delegate.put(validator, resut);
        }

        public Throwable getError() {
            return failCause;
        }

    }

}

This works but it leaves me with a few questions:

1. The very first guideline to API designing says Do not Return Null to Indicate the Absence of a Value, but here I am returning a null object in Result#getError() method.

2. Does the use of generics on my example sound like code smell?

This one is going to be opinion based

3. Is it worthy of making it into an API instead of just 4 classes?

Source code could be found here.

Answer 1

A quick first point I'm going to make is that if you have a function like this:

public float divide(int numerator, int denominator) throws DivideByZeroException {
    if(!isValidDenominator(denominator)) {
        throw new DivideByZeroException();
    }
    return (float)numerator/denominator;
}

private boolean isValidDenominator(int denominator) {
    return denominator != 0;
}

You could simplify things by using this structure instead:

public float divide(int numerator, int denominator) throws DivideByZeroException {
    checkValidDenominator(denominator);
    return (float)numerator/denominator;
}

private void checkValidDenominator(int denominator) throws DivideByZeroException {
    if(denominator == 0){
        throw new DivideByZeroException();
    }
}

Note that you can also easily combine several checks into a single check method:

public int calculateMyThing(DataObject obj) throws InvalidInputException, NoGFException{
    checkValidDataObject();

    // calculate my thing here knowing the DataObject is valid

    return myThing;
}

private void checkValidDataObject(DataObject obj) 
            throws InvalidInputException, NoGFException {
    if(obj == null) {
        throw new InvalidInputException(obj);
    }
    checkValidFoo(obj);
    checkValidBar(obj);
    checkValidGirlfriend(obj);
    ...
}

If you're lucky and multiple methods use the same sequence of checks, you can use the combined check in all of them, greatly reducing the amount of code in your class.

---

As for your validation API. This actually doesn't work that well.

What do you want to do with an Exception? Do you want to throw it?

public int foo(DataObject) throws FooException, NullPointerException {
    Validator<DataObject> fooValidator = ???
    if(!fooValidator.isOk()){
         throw fooValidator.getError() // <- type is `Throwable`
    }
    ...
}

How can the compiler know that fooValidator.getError() is of either FooException or NullPointerException? Are we sure that it's not a BarException?

Another problem I have with this example is that I have no idea where the fooValidator should come from. Do you make an instance of a specific class? How is that better than just calling a specific (static) method?

You actually need more code now than you did before.

---

You are, however, correct in your concern about the big chunk of validation code mixed with business logic. Since you decided that these are 2 separate responsibilities, it's advisable to put the validation into a separate class. But does this in itself imply you need to design an entire API around it? I'd say no.

Instead you should focus on what parts of your code belong together, and what parts can be reused.

Start small. Extract the validation logic from the functions entirely. First by putting them into their own function in the same class. See which functions can be reused, or which can be slightly modified to become reusable in other places.

Next, look at which functions logically belong together. Extract those into their own class.

At this point you can start looking how those validation classes relate to each other. If they all do the same checks, but on a different type of input, then you can look at generics to combine them into a single class.

What you can combine and how/when to do so greatly depends on the exact code you're trying to refactor. Without that code it's pretty hard to give concrete advice.