Design pattern question for maintainability

Question

I'm not sure if there is a pattern that should be used here but here is the situation:

I have a number of concrete classes that implement an interface:

public interface IPerformAction
{
   bool ShouldPerformAction();
   void PerformAction();
}

I have another class that checks input to determine if ShouldPerformAction should be execute. The rub is that new checks are added fairly frequently. The interface for the checking class is defined as follows:

public interface IShouldPerformActionChecker
{
   bool CheckA(string a);
   bool CheckB(string b);
   bool CheckC(int c);
   // etc...
}

Finally I currently have the concrete classes call each of the checker methods with the data specific to that concrete class:

public class ConcreteClass : IPerformAction
{
   public IShouldPerformActionCheck ShouldPerformActionChecker { get; set; }

   public string Property1 { get; set; }
   public string Property2 { get; set; }
   public int Property3 { get; set; }

   public bool ShouldPerformAction()
   {
      return 
         ShouldPerformActionChecker.CheckA(this.Property1) ||
         ShouldPerformActionChecker.CheckB(this.Property2) ||
         ShouldPerformActionChecker.CheckC(this.Property3);
   }

   public void PerformAction()
   {
      // do something class specific
   }
}

Now each time I add a new check, I have to refactor the concrete classes to include the new check. Each concrete class passes different properties to the checking method so subclasses the concrete classes is not an option. Any ideas on how this could be implemented in a cleaner manner?

Answer 1

When you create a new CheckN, you're going to have to implement it in each of your concrete checker classes anyway, no?

Or are you talking about refactoring your IPerformActions to actually call that check?

Why don't you just have a CheckAll that calls everything?

Answer 2

You could consolidate the checks into a single general method that takes an object:

public interface IShouldPerformActionChecker
{
   bool Check(object o);
}

Then have a list of these checks in your concrete class:

public List<IShouldPerformActionCheck> ShouldPerformActionChecker { get; set; }

Less type-safe, but more flexible.

Instead of using IShouldPerformActionCheck you could look at using Predicate<T> delegate, which does roughly the same thing.

Answer 3

Let's take a step back - why are you using interfaces in the first place? Can a single implementation of IShouldPerformActionCheck be shared between multiple implementations of IPerformAction? It seems the answer is no, since ICheck must be aware of implementation-specific properties (Property1, Property2, Property3) on the Action in order to perform the check. Therefore the relationship between IAction and ICheck requires more information than the IAction contract can provide to ICheck. It seems your Check classes should be based on concrete implementations that are coupled to the specific type of action they check, like:

abstract class CheckConcreteClass
{
    abstract void Check(ConcreteClass concreteInstance);
}

Answer 4

Instead of have the concrete classes try to check if the action should be performed, there might be a more maintainable way to have arrange these objects.

What if the checker actually implemented IPerformAction, and had an IPerformAction member that it would call if the action should be performed? That member could either be another checker in the chain, or the actual class that performs the action if all the criteria have been passed?

To do that might require that you refactor slightly, so that the logic to do the action is contained in one class, while the specific data to be used is in another one (kind of like a Command pattern), so that the checkers could do their jobs.

In this way, you could easily add another validation rule, just by putting it into the 'chain' of objects leading to the final action.

Answer 5

The "CheckA", "CheckB", etc. names, presumably chosen to avoid exposing confidential information, also obfuscate the nature of the relationships between classes, so I'll have to wing it.

However, this is very nearly double dispatch, except you are performing conversion of the objects in-between.

EDIT: Try playing the double dispatch pattern "by the book", rather than decomposing the objects mid-dispatch. To do this, you'd need something like the following:

public interface IPerformAction
{
    bool ShouldPerformAction(IShouldPerformActionChecker checker);
    void PerformAction();
}

public interface IShouldPerformActionChecker
{
    bool CheckShouldPerformAction(FloorWax toCheck);
    bool CheckShouldPerformAction(DessertTopping toCheck);
    // etc...
}

public class FloorWax : IPerformAction
{
    public string Fragrance { get; set; }

    // Note that the text of this method is identical in each concrete class,
    // but compiles to call a different overload of CheckShouldPerformAction.
    public bool ShouldPerformAction(IShouldPerformActionChecker checker)
    {
        return checker.CheckShouldPerformAction(this);
    }
}

public class DessertTopping: IPerformAction
{
    public string Flavor { get; set; }

    // Note that the text of this method is identical in each concrete class,
    // but compiles to call a different overload of CheckShouldPerformAction.
    public bool ShouldPerformAction(IShouldPerformActionChecker checker)
    {
        return checker.CheckShouldPerformAction(this);
    }
}

public class ShimmerApplicationChecker : IShouldPerformActionChecker
{
    // handles binding of FloorWax class to required checks
    public bool CheckShouldPerformAction(FloorWax toCheck)
    {
        return CheckAroma(toCheck.Fragrance);
    }

    // handles binding of DessertTopping class to required checks
    public bool CheckShouldPerformAction(DessertTopping toCheck);
    {
        return CheckAroma(toCheck.Flavor);
    }

    // some concrete check
    private bool CheckAroma(string aroma)
    {
        return aroma.Contains("chocolate");
    }
}

Answer 6

You could try something like this:

List<IShouldPerformActionChecker> checkers = new List<IShouldPerformActionChecker>();

//... add in each checker to try

foreach(ConcreteClass objectToCheck in checkset) {
   bool isGood = true;
   foreach(IShouldPerformActionChecker checker in checkers) {
      isGood &= checker.DoCheck(objectToCheck.Property);

       if (!isGood) { break; }
   }

   //handle failed check here
}