Object-oriented – Is Visitor Pattern valid in this scenario

designdesign-patternsobject-orientedvisitor-pattern

The goal of my task is to design a small system which can run scheduled recurring tasks. A recurring task is something like "send an email to administrator every hour from 8:00 am to 5:00 pm, Monday through Friday".

I have a base class called RecurringTask.

public abstract class RecurringTask{

    // I've already figured out this part
    public bool isOccuring(DateTime dateTime){
        // implementation
    }

    // run the task
    public abstract void Run(){

    }
}

And I have several classes which are inherited from RecurringTask. One of them is called SendEmailTask.

public class SendEmailTask : RecurringTask{
    private Email email;

    public SendEmailTask(Email email){
        this.email = email;
    }

    public override void Run(){
        // need to send out email
    }
}

And I have a EmailService which can helps me to send out an email.

The last class is RecurringTaskScheduler, it's responsible for loading tasks from cache or database and run the task.

public class RecurringTaskScheduler{

    public void RunTasks(){
        // Every minute, load all tasks from cache or database
        foreach(RecuringTask task : tasks){
            if(task.isOccuring(Datetime.UtcNow)){
                task.run();
            }
        }
    }
}

Here is my problem: where should I put EmailService?

Option1: Inject EmailService into SendEmailTask

public class SendEmailTask : RecurringTask{
    private Email email;

    public EmailService EmailService{ get; set;}

    public SendEmailTask (Email email, EmailService emailService){
        this.email = email;
        this.EmailService = emailService;
    }

    public override void Run(){
        this.EmailService.send(this.email);
    }
}

There are already some discussions on whether we should inject a service into an entity, and most people agree it's not a good practice. See this article.

Option2: If…Else in RecurringTaskScheduler

public class RecurringTaskScheduler{
    public EmailService EmailService{get;set;}

    public class RecurringTaskScheduler(EmailService emailService){
        this.EmailService = emailService;
    }

    public void RunTasks(){
        // load all tasks from cache or database
        foreach(RecuringTask task : tasks){
            if(task.isOccuring(Datetime.UtcNow)){
                if(task is SendEmailTask){
                    EmailService.send(task.email); // also need to make email public in SendEmailTask
                }
            }
        }
    }
}

I've been told If…Else and cast like above is not OO, and will bring more problems.

Option3: Change the signature of Run and create ServiceBundle.

public class ServiceBundle{
    public EmailService EmailService{get;set}
    public CleanDiskService CleanDiskService{get;set;}
    // and other services for other recurring tasks

}

Inject this class into RecurringTaskScheduler

public class RecurringTaskScheduler{
    public ServiceBundle ServiceBundle{get;set;}

    public class RecurringTaskScheduler(ServiceBundle serviceBundle){
        this.ServiceBundle = ServiceBundle;
    }

    public void RunTasks(){
        // load all tasks from cache or database
        foreach(RecuringTask task : tasks){
            if(task.isOccuring(Datetime.UtcNow)){
                task.run(serviceBundle);
            }
        }
    }
}

The Run method of SendEmailTask would be

public void Run(ServiceBundle serviceBundle){
    serviceBundle.EmailService.send(this.email);
}

I don't see any big problems with this approach.

Option4: Visitor pattern.
The basic idea is create a visitor which will encapsulate services just like ServiceBundle.

public class RunTaskVisitor : RecurringTaskVisitor{
    public EmailService EmailService{get;set;}
    public CleanDiskService CleanDiskService{get;set;}

    public void Visit(SendEmailTask task){
        EmailService.send(task.email);
    }

    public void Visit(ClearDiskTask task){
        //
    }
}

And we also need to change the signature of Run method. The Run method of SendEmailTask is

public void Run(RecurringTaskVisitor visitor){
    visitor.visit(this);
}

It is a typical implementation of Visitor Pattern, and the visitor will be injected into RecurringTaskScheduler.

In summary:
Among these four approaches, which one is the best for my scenario? And are there any big difference between Option3 and Option4 for this problem?

Or do you have better idea on this problem? Thanks!

Update 5/22/2015: I think Andy's answer summarizes my intention really well; if you are still confused about the problem itself, I suggest reading his post first.

I just found out my problem is very similar to Message Dispatch problem, which leads to Option5.

Option5: Convert my problem to Message Dispatch.
There is a one-to-one mapping between my problem and Message Dispatch problem:

Message Dispatcher : Receive IMessage and dispatch sub classes of IMessage to their corresponding handlers. → RecurringTaskScheduler

IMessage : An interface or an abstract class. → RecurringTask

MessageA : Extends from IMessage, having some additional information. → SendEmailTask

MessageB : Another subclass of IMessage. → CleanDiskTask

MessageAHandler : When receive MessageA, handle it → SendEmailTaskHandler, which contains EmailService, and will send out an email when it receives SendEmailTask

MessageBHandler : Same as MessageAHandler, but handle MessageB instead. → CleanDiskTaskHandler

The hardest part is how to dispatch different kind of IMessage to different handlers. Here is a useful link.

I really like this approach, it doesn't pollute my entity with service, and it doesn't have any God class.

Best Answer

I would say Option 1 is the best route to take. The reason you should not dismiss it is that the SendEmailTask is not an entity. An entity is an object concerned with holding data and state. Your class has very little of that. In fact, it is not an entity, but it holds an entity: the Email object you are storing. That means that Email should not take a service, or have a #Send method. Instead, you should have services that take entities, such as your EmailService. So you are already following the idea of keeping services out of entities.

Since SendEmailTask is not an entity, it is therefore perfectly fine to inject the email and the service into it, and that should be done through the constructor. By doing constructor injection, we can be sure that SendEmailTask is always ready to perform it's job.

Now let's look at why not to do the other options (specifically with respect to SOLID).

Option 2

You've been rightly told that branching on type like that will bring more headaches down the road. Let's look at why. First, ifs tend to cluster and grow. Today, it's a task to send emails, tomorrow, every different type of class needs a different service or other behavior. Managing that if statement becomes a nightmare. Since we are branching on type (and in this case explicit type), we are subverting the type system built into our language.

Option 2 is not Single Responsibility (SRP) because the formerly reusable RecurringTaskScheduler now has to know about all these different types of tasks, and about all the different kinds of services and behaviors they might need. That class is much harder to reuse. It is also not Open/Closed (OCP). Because it needs to know about this kind of task or that one (or this kind of service or that one), disparate changes to tasks or services could force changes here. Add a new task? Add a new service? Change the way email is handled? Change RecurringTaskScheduler. Because the type of task matters, it does not adhere to Liskov Substitution (LSP). It can't just get a task and be done. It has to ask for the type and based on the type do this or do that. Rather than encapsulating the differences into the tasks, we are pulling all of that into the RecurringTaskScheduler.

Option 3

Option 3 has some big problems. Even in the article you link to, the author discourages doing this:

You can still use a static service locator…

I avoid service locator when I can, especially when the service locator must be static…

You are creating a service locator with your ServiceBundle class. In this case, it doesn't appear to be static, but it still has many of the problems inherent in a service locator. Your dependencies are now hidden away under this ServiceBundle. If I give you the following API of my cool new task:

class MyCoolNewTask implements RecurringTask
{
    public bool isOccuring(DateTime dateTime) {
        return true; // It's always happenin' here!
    }

    public void Run(ServiceBundle bundle) {
        // yeah, some awesome stuff here
    }
}

What are the services I am using? What services need to be mocked out in a test? What's to stop me from using every service in the system, just because?

If I want to use your task system to run some tasks, I now am dependent on every service in your system, even if I only use a few or even none at all.

The ServiceBundle is not really SRP because it needs to know about every service in your system. It is also not OCP. Adding new services means changes to the ServiceBundle, and changes to the ServiceBundle may mean disparate changes to tasks elsewhere. ServiceBundle does not Segregate its Interface (ISP). It has a sprawling interface of all these services, and because it's just a provider for those services, we could consider its interface to encompass the interfaces of all of the services it provides as well. Tasks no longer adhere to Dependency Inversion (DIP), because their dependencies are obfuscated behind the ServiceBundle. This also does not adhere to the Principle of Least Knowledge (aka the Law of Demeter) because things know about many more things than they have to.

Option 4

Previously, you had lots of small objects that were able to operate independently. Option 4 takes all of these objects and smashes them together into a single Visitor object. This object acts as a god object over all of your tasks. It reduces your RecurringTask objects to anemic shadows that simply call out to a visitor. All of the behavior moves to the Visitor. Need to change behavior? Need to add a new task? Change Visitor.

The more challenging part is that, because all of the different behaviors are all in a single class, changing some polymorphicly drags along all of the other behavior. For example, we want to have two different ways to send email (they should use different servers maybe?). How would we do it? We could create an IVisitor interface and implement that, potentially duplicating code, like #Visit(ClearDiskTask) from our original visitor. Then if we come up with a new way to clear a disk, we have to implement and duplicate again. Then we want both kinds of changes. Implement and duplicate again. These two different, disparate behaviors are inextricably linked.

Maybe instead we could just subclass Visitor? Subclass with new email behavior, subclass with new disk behavior. No duplication so far! Subclass with both? Now one or the other needs to be duplicated (or both if that's your preference).

Let's compare to option 1: We need a new email behavior. We can create a new RecurringTask that does the new behavior, inject in its dependencies, and add it to the collection of tasks in the RecurringTaskScheduler. We don't even need to talk about clearing disks, because that responsibility is somewhere else entirely. We also still have the full array of OO tools at our disposal. We could decorate that task with logging, for example.

Option 1 will give you the least pain, and is the most correct way to handle this situation.

Related Solutions

Visitor Pattern – Implementing for an Abstract Syntax Tree

It is up to the visitor implementation to decide whether to visit child nodes and in which order. That's the whole point of the visitor pattern.

In order to adapt the visitor for more situations it is helpful (and quite common) to use generics like this (it's Java):

public interface ExpressionNodeVisitor<R, P> {
    R visitNumber(NumberNode number, P p);
    R visitBinary(BinaryNode expression, P p);
    // ...
}

And an accept method would look like this:

public interface ExpressionNode extends Node {
    <R, P> R accept(ExpressionNodeVisitor<R, P> visitor, P p);
    // ...
}

This allows to pass additional parameters to visitor and retrieve a result from it. So, the expression evaluation can be implemented like this:

public class EvaluatingVisitor
    implements ExpressionNodeVisitor<Double, Void> {
    public Double visitNumber(NumberNode number, Void p) {
        // Parse the number and return it.
        return Double.valueOf(number.getText());
    }
    public Double visitBinary(BinaryNode binary, Void p) {
        switch (binary.getOperator()) {
        case '+':
            return binary.getLeftOperand().accept(this, p)
                + binary.getRightOperand().accept(this, p);
        // More cases for other operators here.
        }
    }
}

The accept method parameter isn't used in the above example, but just believe me: it is quite useful to have one. For example, it can be a Logger instance to report errors to.

Design Patterns – Communicating Between Unrelated Components

In the example you give (mission controller turns monsters blue), you wouldn't need communication between the controller and the monsters. Instead the monsters could look up the "monster color" in a palette (or color table). The mission controller just sets a different value for "monster color" in the palette when the mission changes. Or switch the entire palette if the mission change affects not only monsters ...

Call the palette a style sheet and you will get the idea that a mission change will be able to change more than just colors ...

However for your other question:

How do you allow communication between two completely unrelated objects, if you don't want them to know about each other, AND not use the Observer pattern?

In my opinion, the best way to achieve this is via events, i.e. in your example:

the monsters are subscribed on an event channel for the 'mission change event'
the controller issues a 'mission change event' on the event channel, and
the monsters change their color when they receive the 'mission change event'

That way both controller and monsters don't have to know about each other. However, both have to know about the (same) 'mission change event' and the (same) event channel.

To achieve a more decoupled design, you could also combine this with the palette approach described above as follows:

a palette (or style sheet) manager is subscribed on an event channel for the 'mission change event'
the controller issues a 'mission change event' on the event channel, and
the manager switches to a different palette (or style sheet) on receiving the 'mission change event'; the palette is chosen based on information transported in the event ...