15

When I have a lot of data that needs to be validated, should I create a new class for the sole purpose of validation or should I stick with in-method validation?

My particular example contemplates a tournament and a event/category class: Tournament and Event, which models a sport tournament and each tournament has one or many categories.

There are all sort of things to validate in these classes: the players should be empty, should be unique, the number of matches each player should play, the number of players each match has, predefined matchups, and a really big etcetera including much more complex rules.

There are also some parts I need to validate as a whole, like how classes integrate with each other. For example, the unitary validation of a Playercan be just fine, but if an event has the same player twice, that's a validation error.

So how about this?: I forget about absolutely any pre-check when using my model classes' setters and similar methods to add data and instead I let validation classes handle that.

So we will have something like EventValidator with an Event as a member, and a validate() method that validates the whole object, plus singular methods to validate all members' rules.

Then, before instantiating a validable object, I will execute the validation to prevent illegal values.

Is my design correct? Should I do something differently?

Also, should I use boolean returning validation methods? Or just throw an exception if the validation fails? It seems to me the best option would be boolean returning methods and throw the exception when the object is instantianted, for example:

public Event() {
    EventValidator eventValidator = new EventValidator(this);
    if (!eventValidator.validate()) {
        // show error messages with methods defined in the validator
        throw new Exception(); // what type of exception would be best? should I create custom ones?
    }
}
7

It is OK to delegate any logic by means of composition if that logic is going to change dynamically during the execution of a program. Complex validations like the ones you explain are as good a candidate as any to be delegated to another class via composition.

Remember though that validations can occur in different moments.

Instantiating a concrete validator like in your example is a bad idea because it couples your Event class to that particular validator.

Let's assume you are not using any DI framework.

You can add the validator to the constructor, or inject it with a setter method. I suggest a creator method in a factory instantiates both the Event and the validator and then passes it either to the event constructor or with a setValidator method.

Obviously a Validator interface and or abstract class should be writen so your classes depend on it and not on any concrete validator.

Executing the validate method in the constructor can be problematic because all the state you want to validate may not be in place yet.

I suggest that you creta a Validable interface and let your classes implement it, that interface could have a validate() method.

That way the upper components of your application call the validate method at will (which in turn is delegated to the validator member).

==> IValidable.java <==

import java.util.List;

public interface IValidable {
    public void setValidator(IValidator<Event> validator_);
    public void validate() throws ValidationException;
    public List<String> getMessages();
}

==> IValidator.java <==

import java.util.List;

public interface IValidator<T> {
    public boolean validate(T e);
    public List<String> getValidationMessages();
}

==> Event.java <==

import java.util.List;

public class Event implements IValidable {

    private IValidator<Event> validator;

    @Override
    public void setValidator(IValidator<Event> validator_) {
        this.validator = validator_;
    }

    @Override
    public void validate() throws ValidationException {
        if (!this.validator.validate(this)){
            throw new ValidationException("WTF!");
        }
    }

    @Override
    public List<String> getMessages() {
        return this.validator.getValidationMessages();
    }

}

==> SimpleEventValidator.java <==

import java.util.ArrayList;
import java.util.List;

public class SimpleEventValidator implements IValidator<Event> {

    private List<String> messages = new ArrayList<String>();
    @Override
    public boolean validate(Event e) {
        // do validations here, by accessing the public getters of e
        // propulate list of messages is necessary
        // this example always returns false    
        return false;
    }

    @Override
    public List<String> getValidationMessages() {
        return this.messages;
    }

}

==> ValidationException.java <==

public class ValidationException extends Exception {
    public ValidationException(String message) {
        super(message);
    }

    private static final long serialVersionUID = 1L;
}

==> Test.java <==

public class Test {
    public static void main (String args[]){
        Event e = new Event();
        IValidator<Event> v = new SimpleEventValidator();
        e.setValidator(v);
        // set other thins to e like
        // e.setPlayers(player1,player2,player3)
        // e.setNumberOfMatches(3);
        // etc
        try {
            e.validate();
        } catch (ValidationException e1) {
            System.out.println("Your event doesn't comply with the federation regulations for the following reasons: ");
            for (String s: e.getMessages()){
                System.out.println(s);
            }
        }
    }
}
  • So in terms of class hierarchy this doesn't differ much from what I suggested, does it? Instead of instantiating and calling the validator inside the constructor, it'd be created and called when needed in the execution flow, this is the main difference I can see. – dabadaba Apr 16 '16 at 22:45
  • My answer is basically that it's OK to create another class for doing complex validation. I just added some advice for you to avoid hard coupling and get more flexibility. – Tulains Córdova Apr 16 '16 at 23:22
  • If I were to add error messages in a list or dictioary, should it be in Validator or in Validable? And how can I work these messages in conjunction with the ValidationException? – dabadaba Apr 21 '16 at 16:48
  • 1
    @dabadaba The list should be a member of IValidator implementors, but IValidable should add access to that method so implementors delegate it. I assumed more than one validation message can be returned. Clic on the edited n minutes ago link at the bottom so you can see the differences. It's better if you use the side-by-side view (no markdown). – Tulains Córdova Apr 21 '16 at 18:57
  • 3
    Probably being pedantic, but I assume Validatable is a far better name than Validable as it is the real adjective – user919426 May 17 '16 at 9:10
4

I forget about absolutely any pre-check when using my model classes' setters and similar methods to add data

That's the problem. Ideally you should prevent your objects to have invalid state: Don't allow instantiation with invalid state and if you must have setters and other state-changing methods, throw an exception right there.

instead I let validation classes handle the validation.

This is not contradictionary. If the validation logic is complex enough to warrant its own class, you can still delegate validation. And if I understand you correct, this is exactly what you are doing now:

Then, before instantiating a validable object, I will execute the validation to prevent illegal values.

If you still have setters that might result in invalid state, make sure to validate before actually changing state. Otherwise you'll have an error message but still leave your object with invalid state. Again: prevent your objects to have invalid state

Also, should I use boolean returning validation methods? Or just throw an exception if the validation fails? It seems to me the best option would be boolean returning methods and throw the exception when the object is instantianted

Sounds good to me, as the validator expects valid or invalid input so from its perspective, invalid input is not an exception.

Update: If "the system as a whole" becomes invalid, the reason is that some object must have changed (e.g. a player) and some possibly higher level object (e.g. an event) that references this object has a condition that is not fulfilled anymore. Now a solution would be to not allow direct changes to the player that could make something invalid at a higher level. This is where immutable objects help. Then a changed player is a different object. Once a player is associated to an event, you can only change it from the event itself (because you'll have to change the reference to a new object) and again validate immediately.

  • What about having no checks whatsoever in the instantiation and setters and having the validation as an aside operation? Something like Tulains Córdova suggested. Because validating and throwing an exception in a setter or constructor might work for that member or object, but it doesn't help check if the system as a whole is ok. – dabadaba Apr 17 '16 at 14:51
  • @dabadaba my answer was to long for a comment, please see update above – Fabian Schmengler Apr 17 '16 at 20:56
  • I don't want to use immutable objects though, I want to be able to modify my classes. And I don't know how to make immutable objects, unless you're referring to the final keyword. In that case I would have to change my design a lot because I am partially constructing events and tournaments with additional setters (because this data is aditional and configurable). – dabadaba Apr 17 '16 at 21:13
  • 3
    To make immutable objects, remove all setters and only use constructors to set values. If you have optional data or data that's not available at once, consider using the builder pattern: developer.amd.com/community/blog/2009/02/06/… – Fabian Schmengler Apr 17 '16 at 21:19

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.