2

I want to add capabilities for an abstract Model class to send events whenever it updates. To be able to send events, it needs to have access to an EventManager. So, the intent were first, to be able to handle events (e.g. save operation) and second to send Events to the system event handler.

My approach is to implement a new interface EventManagerAware in ExistingModel class, which needs this behaviour, like this:

interface EventManagerAware {

    public function handleEvent(EventManager em);
}

class ExistingModel extends Model implements EventManagerAware {

    public function handleEvent(EventManager em) {
        em.sendEvent(Event::SAVE);
    }
}

abstract class Model() {

    public function save() {

        if (this instanceOf EventManagerAware) {
            this.handleEvent(this.getServiceManager().getEventManager());
        }
}

After review, someone argued that it's better to just extend the Model class, override the save() method and change ExistingModel from extending Model to EventPublisherModel class, like so:

class EventPublisherModel extends Model {

    public function save() {
        this.getServiceManager().getEventManager().sendEvent(Event::SAVE);
    }
}

class ExistingModel extends EventPublisherModel {

}

FYI, the existing Model class looks like this:

abstract class Model {
    protected ServiceManager serviceManager;

    public function __construct(ServiceManager sm) {
        this.serviceManager = sm;
    }

    public function getServiceManager() {
        return this.serviceManager;
    }

    public function save() {

    }
}

One of the arguments put for having EventPublisherModel class was because doing in the first approach, all Model objects would check for EventManagerService when doing save() operation, and that's unnecessary.

I've never really thought about why the first approach would be better, but did think that it I have Segregated the Interface and didn't think inheritance is needed because sending events is in different domain from the Model class.

Which is the best way of doing this and why?

  • 3
    Second is better, but you should go one step further and get rid of service locator (anti-pattern). Inject EventManager via constructor of ExistingModel and override Model.save(). You might also search the web for decorator pattern information. – shudder Oct 6 '15 at 7:55
  • From your definition of Model, it sounds like in your system a "model" is something which can be saved, and which can provide a service manager. That seems a little weird. Could you explain what you mean by sending events being in a different domain from the model class? – Ben Aaronson Oct 6 '15 at 9:16
  • @shudder I added existing constructor in Model which already doing what you suggested. Would be great if you can give an answer with explanation why you think it's better. – imel96 Oct 6 '15 at 11:33
  • @BenAaronson it is weird. Model here is basically an ActiveRecord stuffed with high level logic that lots of time needs other object to do its job, that's why it's armed with a service manager. I mean sending events would be more like logging, which is in system domain rather than in this business logic heavy Model. – imel96 Oct 6 '15 at 11:46
  • 1
    The instanceOf conditional operation is a code smell that suggests a more natural object oriented technique is likely. – Erik Eidt Oct 6 '15 at 16:17
3

First, your method naming can be improved. handleEvent should better be called sendSaveEvent, because it's responsibility is not to "handle" any event, it is for sending events, and it is not for sending generic events, only for "save" events.

Second, your example does not show the actual "saving" code. If you had added a placeholder for this, it becomes obvious why the second solution is not really a good idea - when each subclass needs to override the save method just for adding "even sending logic", the result is at least some duplicated code. This is not necessary, the code can be kept in one place (in the abstract model base class).

This in mind, I would consider to solve this with the template method pattern:

abstract class Model{

    public function save() {
       // ... here is the actual saving code ...

       sendSaveEvent(this.getServiceManager().getEventManager());
    }
    public function sendSaveEvent(EventManager em) {
          // empty default implementation
    }
}

and in the derived class:

class ExistingModel extends Model {

    // overridden function
    public function sendSaveEvent(EventManager em) {
        em.sendEvent(Event::SAVE);
    }
}

As you see, the code is simpler, the EventManagerAware interface is not necessary any more, the EventPublisherModel is not necessary, and each derivation of Model can simply choose if and how to send a SAVE event.

UPDATE: using dependency injection together with the null object pattern can make your desing even more simple:

  class Model  // maybe abstract
  {

    // Initialize this variable with a value by constructor 
    // injection either directly in the Model class, 
    // or in a subclass, whatever suits your needs.
    EventManager _myEventManager;

    public Model()
    {
        // The "EmptyEventManager" has an empty "sendEvent" method
        // (some kind of "null" object).    
        _myEventManager = new EmptyEventManager()
    }

    public Model(EventManager em)
    {
       _myEventManager=em;
    }

    public function save() {
       // ... here is the actual saving code ...

       _myEventManager.sendEvent(Event::SAVE); // sendEvent is a virtual method
    }
}

This does not only eliminate your "service locator", it also allows you to switch between "no event sending" and "event sending" just by providing a different EventManager subclass to your Model object (without the need for introducing a class like "ExistingModel", but if you need this subclass for different reasons, you can still add it, if you like). The pattern applied is is the well known strategy pattern, with the EventManager as strategy object.

As a final remark: I do not know your "real" use case, but would it not make more sense to provide a default implementation in the Model base class where the SAVE event is actually sended?

  • It was called sendSaveEvent(), then I changed it to handleEvent() to make it easier to compare with the other approach. I agree that this solution is clearer and direct to the point. Then I realised DI is probably even better to pass the EventManager. – imel96 Dec 7 '15 at 1:49
  • @imel96: see my edit – Doc Brown Dec 7 '15 at 8:33
  • Thanks. The reason I don't want to have default implementation is only 2% of classes needs it. Also, it'll add more work to the Model class. Already, this Model class is really a "mutable model (active record) that can log, translate strings, store database meta, factory, holds business logic..." and so on. I can't really fix it, but also don't want to make it worse. – imel96 Dec 7 '15 at 23:47
  • Your question, as it is written, provides a default "do nothing" implementation, that's why I provided one, too. No one forces you to do so, let your program crash with an error if you prefer that (that can be a serious approach, but that was not what you asked for). And when you derive a class from a god class, what you get is not a separation of concerns, you get two god classes. Better use you Model class as a facade, and delegate the work to classes like the Event Manager or other strategy objects. – Doc Brown Dec 8 '15 at 5:41
0

Which way is best depends on your needs. With the first way, each class will have identical, or nearly identical, code for handling events. This causes code duplication. Each class can easily handle the events in its own way. It is possible to add event manager awareness further along the inheritance chain. Another negative is the type check for every Model based class in save.

In the second way the code for handling events is in one place. You don't need to add it to every class. Types that don't need event handling are unaffected by it. Models that need custom event handling can create their own implementation of save. Event manager awareness will need to be added to classes at the point they derive from Model. Overall the code will be simpler and easier to maintain.

In summary, the second (EventPublisher) way is usually the way to go.

0

It seems like the sending of events is a secondary concern for your Model class, so why not make it the job of a separate class entirely?

public interface Model {
    void save();
}

public class ExistingModel implements Model {
    public void save() {
        //save stuff
    }
}

public class EventPublishingModel implements Model {
    private Model delegate;
    private EventManager events;

    public EventPublishingModel(Model delegate, EventManager events) {
        this.delegate = delegate;
        this.events = events;
    }

    public void save() {
        delegate.save();
        events.sendEvent(Event::SAVE);
    }

    //other methods can just call the delegate
}

And then you can have the simple model class, without any changes:

 Model model = new ExistingModel();

But decorate it with the event-publishing functionality where appropriate:

 model = new EventPublishingModel(model, eventManager);
  • This gives you additionally flexibility for the price of a more complicated implementation - if the flexibility (having ExistingModel objects with and without event sending) is not needed, this is more complicated than necessary. – Doc Brown Nov 6 '15 at 6:33

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.