Is Visitor Pattern valid in this scenario?

Question

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.

Answer 1

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.

Answer 2

Have you had a look at existing libraries e.g. spring quartz or spring batch (I'm not sure what fits your needs most)?

To your question:

I assume the problem is, that you want to persist some metadata to the task in a polymorphic way, so an e-mail task has e-mail addresses assigned, a log-task a log-level, and so on. You can store a list of those in memory or in your database but to seperate concerns you don't want to have the entity polluted with service-code.

My proposed solution:

I would separate the running- and the data-part of the task, to have e.g. TaskDefinition and a TaskRunner. The TaskDefinition has a reference to a TaskRunner or a factory that creates one (e.g. if some setup is required like the smtp-host). The factory is a specific one - it only can handle EMailTaskDefinitions and returns only instances of EMailTaskRunners. This way it is more OO and change safe - if you introduce a new task-type you have to introduce a new specific factory (or reuse one), if you don't you can't compile.

This way you would end up with a dependency: entity layer --> service layer and back again, because the Runner needs information stored in the entity and probably wants to make an update to its state in the DB.

You could break the circle by using a generic factory, which takes a TaskDefinition and returns a specific TaskRunner, but that would require lots of ifs. You could use reflection to find a runner that is similarly named as your definition, but be cautious this approach can cost some performance, and may lead to runtime errors.

P.S. I'm assuming Java here. I think it's similar in .net. The main problem here is the double binding.

To the visitor pattern

I think it was rather intended to be used to exchange an algorithm for different kinds of data objects at runtime, than for pure double binding purposes. For example if you have different kinds of insurances and different kinds of calculating them, e.g. because different countries require it. Then you choose a specific calculation method and apply it on several insurances.

In your case you would choose a specific task-strategy (e.g. email) and apply it to all of your tasks, which is wrong because not all of them are email tasks.

P.S. I didn't test it, but I think your Option 4 won't work either, because it is double-binding again.

Answer 3

I completely disagree with that article. Services (concretely their "API") are important party of the Business Domain and as such will exist within Domain Model. And there is no problem with entities in business domain referencing something else in same business domain.

When X send mail to Y.

Is a business rule. And to do that, service that sends mail is needed. And entity that handles When X should know about this service.

But there are some problems with implementation. It should be transparent to user of the entity, that the entity is using a service. So adding the service in constructor is not a good thing. This is also an issue, when you are deserializing the entity from database, because you need to set both the data of the entity and instances of services. Best solution I can think of is to use property injection after the entity was created. Maybe forcing each newly created instance of any entity to go through "initialize" method that injects all entities the entity needs.

Answer 4

That is a great question and an interesting problem. I propose that you use a combination of Chain of Responsibility and Double Dispatch patterns (pattern examples here).

First lets define the task hierarchy. Notice that there are now multiple run methods to implement the Double Dispatch.

public abstract class RecurringTask {

    public abstract boolean isOccuring(Date date);

    public boolean run(EmailService emailService) {
        return false;
    }

    public boolean run(ExecuteService executeService) {
        return false;
    }
}

public class SendEmailTask extends RecurringTask {

    private String email;

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

    @Override
    public boolean isOccuring(Date date) {
        return true;
    }

    @Override
    public boolean run(EmailService emailService) {
        emailService.runTask(this);
        return true;
    }

    public String getEmail() {
        return email;
    }
}

public class ExecuteTask extends RecurringTask {

    private String program;

    public ExecuteTask(String program) {
        this.program = program;
    }

    @Override
    public boolean isOccuring(Date date) {
        return true;
    }

    public String getName() {
        return program;
    }

    @Override
    public boolean run(ExecuteService executeService) {
        executeService.runTask(this);
        return true;
    }
}

Next lets define the Service hierarchy. We will use Services to form the Chain of Responsibility.

public abstract class Service {

    private Service next;

    public Service(Service next) {
        this.next = next;
    }

    public void handleRecurringTask(RecurringTask req) {
        if (next != null) {
            next.handleRecurringTask(req);
        }
    }
}

public class ExecuteService extends Service {

    public ExecuteService(Service next) {
        super(next);
    }

    void runTask(ExecuteTask task) {
        System.out.println(String.format("%s running %s with content '%s'", this.getClass().getSimpleName(),
                task.getClass().getSimpleName(), task.getName()));
    }

    public void handleRecurringTask(RecurringTask req) {
        if (!req.run(this)) {
            super.handleRecurringTask(req);
        }
    }
}

public class EmailService extends Service {

    public EmailService(Service next) {
        super(next);
    }

    public void runTask(SendEmailTask task) {
        System.out.println(String.format("%s running %s with content '%s'", this.getClass().getSimpleName(),
                task.getClass().getSimpleName(), task.getEmail()));
    }

    public void handleRecurringTask(RecurringTask req) {
        if (!req.run(this)) {
            super.handleRecurringTask(req);
        }
    }
}

The final piece is the RecurringTaskScheduler which orchestrates the loading and running process.

public class RecurringTaskScheduler{

    private List<RecurringTask> tasks = new ArrayList<>();

    private Service chain;

    public RecurringTaskScheduler() {
        chain = new EmailService(new ExecuteService(null));
    }

    public void loadTasks() {
        tasks.add(new SendEmailTask("here comes the first email"));
        tasks.add(new SendEmailTask("here is the second email"));
        tasks.add(new ExecuteTask("/root/python"));
        tasks.add(new ExecuteTask("/bin/cat"));
        tasks.add(new SendEmailTask("here is the third email"));
        tasks.add(new ExecuteTask("/bin/grep"));
    }

    public void runTasks(){
        for (RecurringTask task : tasks) {
            if (task.isOccuring(new Date())) {
                chain.handleRecurringTask(task);
            }
        }
    }
}

Now, here is the example application demonstrating the system.

public class App {

    public static void main(String[] args) {
        RecurringTaskScheduler scheduler = new RecurringTaskScheduler();
        scheduler.loadTasks();
        scheduler.runTasks();
    }
}

Running the application outputs:

EmailService running SendEmailTask with content 'here comes the first email'
EmailService running SendEmailTask with content 'here is the second email'
ExecuteService running ExecuteTask with content '/root/python'
ExecuteService running ExecuteTask with content '/bin/cat'
EmailService running SendEmailTask with content 'here is the third email'
ExecuteService running ExecuteTask with content '/bin/grep'