Composite Strategy pattern - java - How bad is this code?

Question

This question is kind of continuation to my earlier post: Visitor pattern implementation in java- How does this look?

I got a bit confused while refactoring my code. I am trying to convert my visitor pattern (explained in the prior post) into a composite strategy pattern. I am trying to do something like this:

public interface Rule {
  public List<ValidatonError> check(Validatable validatable);
}

Now, I would define a Rule like this:

public class ValidCountryRule  {
  public List<ValidationError> check(Validatable validatable) {
    // invokeDAO and do something, if violation met
    // add to a list of ValidationErrors.
    // return the list.
  }
}

Now, I could have two different types objects to be validated. These two could be completely different: Say I have a Store that is Validatable, and then a Schedule which is Validatable. Now, if I would write a composite that would look like this:

class Validator implements Rule {
  private List<Rule> tests = new ArrayList<Rule>();

  public void addRule(Rule rule) {
    tests.add(rule);
  }

  public List<ValidationError> check(Visitable visitable) {
    List<ValidationError> list = new ArrayList<ValidationError>();
    for(Rule rule : tests) {
      list.addAll(rule.check(visitable);
    }
  }

  public Validator(ValidatorType type) {
    this.tests = type.getRules();
  }
}

I would define an enum that defines what set of checks go where...

public Enum ValidatorType {
  public abstract List<Rule> getRules();
  STORE_VALIDATOR {
    public List<Rule> getRules() {
      List<Rule> rules = new ArrayList<Rule>();
      rules.add(new ValidCountryRule());
      rules.add(new ValidXYZRule());
    }

  // more validators
}

and finally, I would use it like this:

Validator validator = new Validator(ValidatorType.STORE_VALIDATOR);
for (Store store : stores) {
  validator.check(store);
}

I have a strange feeling that my design is flawed. I am not liking the idea that my Rule interface expects a Validatable. Could you please suggest how I would improve this?

Appreciate your help.

Answer 1

When I first learned about design patterns, I kept trying to find places to use them. I've since learned that premature "patternization" is kind of like premature optimization. First, try to do it in a straight-forward way, and then see what problems that gives you.

Try to design with minimal interfaces and subclassing. Then apply whatever pattern may be appropriate for the obvious redundancies you find. I get the impression from this and the previous post that you may be over-architecting your code.

Answer 2

Replace Validatable by a generic type parameter T to make the validation framework type safe.

public interface Rule<T> {
    public List<ValidationError> check(T value);
}

Let's extend our framework with an interface ValidationStrategy:

public interface ValidationStrategy<T> {
    public List<Rule<? super T>> getRules();
}

We are dealing with rules bounded by "? super T" so we can add a rule for Animal to a Dog Validator (assuming Dog extends Animal). The Validator now looks like this:

public class Validator<T> implements Rule<T> {
    private List<Rule<? super T>> tests = new ArrayList<Rule<? super T>>();

    public Validator(ValidationStrategy<T> type) {
        this.tests = type.getRules();
    }

    public void addRule(Rule<? super T> rule) {
        tests.add(rule);
    }

    public List<ValidationError> check(T value) {
        List<ValidationError> list = new ArrayList<ValidationError>();
        for (Rule<? super T> rule : tests) {
            list.addAll(rule.check(value));
        }
        return list;
    }
}

Now we can implement a sample DogValidationStrategy like this:

public class DogValidationStrategy implements ValidationStrategy<Dog> {
    public List<Rule<? super Dog>> getRules() {
        List<Rule<? super Dog>> rules = new ArrayList<Rule<? super Dog>>();
        rules.add(new Rule<Dog>() {
            public List<ValidationError> check(Dog dog) {
                // dog check...
                return Collections.emptyList();
            }
        });
        rules.add(new Rule<Animal>() {
            public List<ValidationError> check(Animal animal) {
                // animal check...
                return Collections.emptyList();
            }
        });
        return rules;
    }
}

Or, like in your sample, we may have an Enum providing several dog validation strategies:

public enum DogValidationType implements ValidationStrategy<Dog> {
    STRATEGY_1 {
        public List<Rule<? super Dog>> getRules() {
            // answer rules...
        }
    },
    // more dog validation strategies
}