Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I was trying to explain the mediator pattern to a new developer, and ended up writing a simple event mediator. Thoughts?

EventMediator

class EventMediator {
    private $events = array();

    public function attach($event, Closure $callback) {
        if(!is_string($event)) throw new \InvalidArgumentException();

        $event = strtolower(trim($event));
        if(!isset($this->events[$event])) $this->events[$event] = array();

        $this->events[$event][] = $callback;

        return $this;
    }

    public function trigger($event, $data = null) {
        if(!is_string($event)) throw new \InvalidArgumentException();

        $event = strtolower(trim($event));
        if(!isset($this->events[$event])) return false;

        foreach($this->events[$event] as $callback) $callback($event, $data);

        return true;
    }

    public function getEvents() { return $this->events; }
}

EventfulTrait

trait EventfulTrait {

    private $eventMediator;

    public function attachEventMediator(EventMediator $eventMediator) {
        $this->eventMediator = $eventMediator;
        return $this;
    }

    public function detachEventMediator() {
        $this->eventMediator = null;
        return $this;
    }

    public function getEventMediator() {
        return $this->eventMediator;
    }

    public function attachEvent($name, $callback) {
        if(!is_null($this->eventMediator)) $this->eventMediator->attach($name, $callback);
        return $this;
    }

    public function triggerEvent($name, $data = null) {
        if(!is_null($this->eventMediator)) return $this->eventMediator->trigger($name, $data);
        return false;
    }
}

Example

require "EventMediator.php";
require "EventfulTrait.php";

class Log {
    public function write($message) {
        echo "{$message}\n";
    }
}

class Eventful {
    use EventfulTrait;

    private $log;

    public function __construct() { $this->log = new Log(); }

    public function attachLoadEvent() {
        $self = $this;
        $log  = $this->log;
        $this->attachEvent(
            "load", function() use($self, $log) {
                $self->runOutOfNames();
                $log->write("log message");
            }
        );
    }

    public function doSomething() {
        $this->triggerEvent("load");
        $this->triggerEvent("event1");
    }

    public function doSomethingElse() {
        $this->triggerEvent("load");
        $this->triggerEvent("event1");
        $this->triggerEvent("event2");
    }

    public function runOutOfNames() { echo "load\n"; }
}

$eventMediator = new EventMediator();
$eventMediator->attach("event1", function() { echo "event1\n"; });
$eventMediator->attach("event2", function() { echo "event2\n"; });
$eventMediator->attach("done", function() { echo "done\n"; });

$eventful = new Eventful();
$eventful->attachEventMediator($eventMediator);
$eventful->attachLoadEvent();
$eventful->doSomething();
$eventful->doSomethingElse();

$eventMediator->trigger("load");
$eventMediator->trigger("done");

This will echo:

load
log message
event1
load
log message
event1
event2
load
log message
done
share|improve this question
add comment

1 Answer 1

I think this is a very clean looking code base, so congrats getting that down. I also think that the the way you implemented the pattern worked out very well.

I don't have much to say, it's mostly just formatting.

  • Throughout, you insist on keeping block sections condensed to one line. It's perfectly okay to add the brackets and break apart the code into multiple lines.
  • In attach(), you return the current object. I don't see why you're doing so as in the example you didn't utilize this feature. Don't write code that's not in use yet!
  • You might consider moving strtolower(trim($event)) to a function with the appropriate parameters. Don't repeat yourself.
  • Why is !isset($this->events[$event]) returning false. I would expect an exception to be throw, that way if the code needs to watch for other errors later on, false could mean many things.
  • attachEventMediator I think needs a new name. You're not just attaching a mediator, you're setting it. If you had support for multiple mediators, then attach would be appropriate. Similarly, detach should be remove or delete.
  • Perhaps in replace of echo "{$message}\n";, you could use the sprintf function. I personally think this would clear up the line.
  • To improve your dependency injection in Eventful, pass the Log as a parameter.
  • Will the message always be "log message"? Maybe pass that as an argument.

Overall I think the code is well-written. A couple more comments and better example code could have improved it even more!

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.