5
\$\begingroup\$

I have the following code which is responsible for the validation in our app (just an example).

Based on the particular tab where user can add person to the database we need to perform additional validation (more fields in each tab). Fields that always should be validated are name and surname so we provided an abstract class with subclasses that are specialized for each tab (i.e. Sourcing). Is it somehow possible to simplify this code by using some design patterns or something ?

abstract public class AbstractPersonValidator {

    public boolean isInvalid( Person person ) {
        if ( person.getName() == null || person.getName().length() < 3 )
            return true;

        if ( person.getSurname() == null || person.getSurname().length() < 3 )
            return true;

        return false;
    }
}

Extending:

public class SourcingPersonValidator extends AbstractPersonValidator {
    @Override
    public boolean isInvalid( Person person ) {
        if ( super.isInvalid( person ) )
            return true;

        if ( person.getTechnology() == null )
            return true;

        if ( person.getSource() == null )
            return true;

        return false;
    }
}
\$\endgroup\$
10
\$\begingroup\$

There are two suggestions I have for situations like this:

  1. double-negatives are bad
  2. use abstract methods for extensions

Let me explain. A method isInvalid(...) is really is not valid, and that can then be used in an expression !isInvalid(...) which is not is not valid... which is complicated. You should reverse the logic in your code, and call the method isValid(...)

That way, your code keeps a better 'read' to it.

The second issue is about how it is best to manage having some base functionality, and extending it for particular purposes. The way I prefer (and I am not sure if there is a formal 'pattern' for this process), is to have the base class do the actual work, and delegate to the specific class for the more fine-grained validation. Your base class would be (note the final on the isValid method....:

abstract public class AbstractPersonValidator {

    public final boolean isValid( Person person ) {
        if ( person.getName() == null || person.getName().length() < 3 ) {
            return false;
        }

        if ( person.getSurname() == null || person.getSurname().length() < 3 ) {
            return false;
        }

        return detailIsValid(person);
    }

    protected abstract boolean detailIsValid();

}

So, that class declares an abstract method that the child classes will need to implement, and the abstract method will do the child-specific checks:

public class SourcingPersonValidator extends AbstractPersonValidator {
    @Override
    protected boolean detailIsValid( Person person ) {
        if ( person.getTechnology() == null ) {
            return false;
        }

        if ( person.getSource() == null ) {
            return false;
        }

        return true;
    }
}

The advantage of doing things this way is that you don't run the risk of a bug where you forget to call super.

Note also that I added braces to your 1-liners. My experience suggests this makes for more reliable code when your code enters a maintenance cycle.

\$\endgroup\$
  • 3
    \$\begingroup\$ That has a name, it's the strategy pattern. \$\endgroup\$ – cbojar Apr 17 '15 at 15:50
7
\$\begingroup\$

@rolfl already addressed some good points, so I just wanted to point out one general thing about name validation.

    public final boolean isValid( Person person ) {
        if ( person.getName() == null || person.getName().length() < 3 ) {
            return false;
        }

Not all names fit nicely into the most common names. What if the name you are trying to validate was Fu Li, or 戴维德? Those would both return false to isValid, even though they are completely real, valid names.

I think you need to consider more edge cases when dealing with real-world arbitrary data.

\$\endgroup\$
4
\$\begingroup\$

Based on the particular tab where user can add person to the database we need to perform additional validation (more fields in each tab). Fields that always should be validated are name and surname so we provided an abstract class with subclasses that are specialized for each tab (i.e. Sourcing). Is it somehow possible to simplify this code by using some design patterns or something ?

One thing to consider is that each of these validators is a distinct implementation of a common interface

public interface PersonValidationRule {
    boolean isValid( Person person );
}

To validate a person, you enumerate a appropriate collection of validation rules, looking for problems

public class PersonValidator {
    private final Collection<PersonValidationRule> rules;

    public final boolean isValid(Person p) {
        for(PersonValidationRule rule : rules) {
            if ( ! rule.isValid(p) ) {
                return false;
            }
        }
        return true;
    }

Different tabs each have an instance of the PersonValidator, with a different collection of rules. You can then define the specific rules that get shared by everybody, and create a default list of rules (perhaps with a Builder) that can be supplemented with additional rules specific to each tab.

\$\endgroup\$

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.