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've taken a look at the SPL Listener/Observer model but found that it doesn't work when using static methods. So I wrote my own but it's very simply. Can anyone suggest ways to make it more like the Model standards and if there is anything obvious that I've missed?

<?php
namespace modular\core;

class Listeners{
    private static $listeners = array();
    private static $triggered = array();
    private static $broadcasts = array();

    public static function attach($event, $key, $callback){
        $config = \modular\core\ModuleLoader::getModuleConfig($key);
        $config['instance'] = get_class($callback[0]);
        $callback['config'] = $config;
        static::$listeners[$event][md5($event.$key)] = array('module' => $key, 'callback' => $callback);
    }

    public static function detach($event, $key, $callback){
        unset(static::$listeners[$event][md5($event.$key)]);
    }

    public static function listeners($event = ''){
        return ($event == '') ? static::$listeners : static::$listeners[$event];
    }

    public static function listenersRegistered(){
        return static::$listeners;
    }

    public static function listenersTriggered(){
        return static::$triggered;
    }

    public static function getBroadcasts(){
        return static::$broadcasts;
    }

    public static function broadcast($event){
        $rendered = \modular\core\Renderer::rendered();

        $caller = debug_backtrace();//true, 1 - check version and implement this
        array_shift($caller);

        static::$broadcasts[$event][] = $caller;

        if(isset(static::$listeners[$event])){
            if(!$rendered)
                ob_start();

            foreach(static::$listeners[$event] as $listener){
                $myEvent = (\modular\core\Router::routed() ? ($listener['module'] == \modular\core\ModuleLoader::getActiveModule()) : true);
                if($myEvent){
                    static::$triggered[$event] = $listener;
                    $test = \modular\core\ModuleLoader::runModule($listener);
                }
            }

            if(!$rendered)
                ob_end_clean();
        }
    }
}
?>

Edit To answer some of the questions asked;

The code to add a listener:

public static function addListener($event, $callback){
        $modules = \modular\core\ModuleLoader::getModules();

        if(($key = array_search($callback[0], $modules)) !== false){
            \modular\core\Listeners::attach($event, $key, $callback);
        }
        else{
            //problem - object not found...
        }
    }

_::addListener('pre-render', array(&$this, 'init'));

Callback is a callback function set as an array to specify the instance

The MD5 is used to combine both the $event and $key as a $key (module) can register to multiple events and vice-versa.

Broadcast is purely checking if any output has already occurred, if not then block all output if so then allow output whilst it calls the module callback method.

Because it's a framework static methods are used as my preferred method to access a class from multiple classes without having to worry about creating variables of instances that I either have to remember globally or re-instantiating the class constantly.

share|improve this question
1  
Question (maybe its just me but i want to help you out): are you asking to turn your static class into a more traditional OOP class since you are using too many static classes? –  azngunit81 Mar 17 at 18:19
    
@azngunit81 given that the Observer Pattern and design patterns in general are something that makes elements of reusable object-oriented software, I think an answer that approaches the OP's code from an OOP perspective would be very much valuable. –  Mat's Mug Mar 17 at 18:24
    
@Mat'sMug right which is why before I started to chop his code up and render it as OOP I just wanted to know if thats what he wanted because in his edit he likes static methods but what he describes can be remedy by using a SOLID approach and have the D part handle with a IoC so that he doesn't need to worry about redefining classes so much –  azngunit81 Mar 17 at 18:40

3 Answers 3

Because it's a framework static methods are used as my preferred method to access a class from multiple classes without having to worry about creating variables of instances that I either have to remember globally or re-instantiating the class constantly.

I don't do , but I'll give this one a try.

Being a framework shouldn't be a reason to prefer static methods everywhere.

The Observer Pattern can be depicted as follows:

Observer pattern

I've taken a look at the SPL Listener/Observer model but found that it doesn't work when using static methods.

There's a reason for that. If I read the code correctly, then when you have a class called SubjectA, and register 3 observers statically, every single instance of SubjectA will have the 3 observers registered; this is unlikely to be the intended behavior (not to mention threading issues), and it's only happening because of the static stuff.1

It looks like your Listeners class is intended to be used in object composition, like, you'll create a Car class that has a Listeners instance, and you'll register listeners for an Explode event. Then you might have a Dog class that has a[nother] Listeners instance, and you'll register listeners for a Bark event. If I assessed the situation correctly, that will give you a barking Car and an exploding Dog, on top of the expected events.

1 that is, if static in has the same meaning as static in , which I'm assuming here.

share|improve this answer
1  
I'm always surprise when my car barks! –  Marc-Andre Mar 17 at 19:39

I see several problems here:

  • It's hard to understand your code because you have not documented any of the methods and their arguments. I have trouble understanding what's the purpose of $event, $key and $callback arguments. First off I though that $callback must be a the callback function, but from code it looks like it's an array of some sort. What should it contain?

  • I don't understand why the md5() function is used. Why don't you just index by $key?

  • The broadcast() seems to be doing some rendering and output buffering. Looks like too many responsibilities for a Listeners class that should be dealing only with event delegation.

  • There are static references to several classes like \modular\core\ModuleLoader. Which means your Listeners class is very tightly coupled with these. It also doesn't help my understanding of your code, as I don't have access to these classes.

  • The fact that you have a need for a static Listeners class feels like a design smell. Too many static methods usually means you aren't really doing proper object-oriented programming, but rather just using classes as containers for your functions. Instead you should consider implementing it as a simple class with normal methods and just instantiating it as a globally available singleton.

share|improve this answer
    
Have edited to clarify, let me know if it helps –  Dean Whitehouse Mar 17 at 17:53

$broadcasts are just for debugging purposes, right?

My thoughts:

  • The name Listeners sounds to me like it's an enhanced homogenous collection. Considering a core part is broadcasting I would suggest renaming. I believe Doctrine2 uses name EventManager, so maybe something along those lines

  • As suggested I would consider moving away from static-ness. You can arguably keep your static method ::addListener() and have an instance of Listeners as a static property if you really do want to keep the static access. But in a well-designed application (where classes have reasonable number of dependencies) handling instances shouldn't be an issue. The class will be more flexible and easier to test (for that matter I wouldn't even recommend the class itself as singleton, having just one instance can be handled as noted above). Take a look at Misko Hevery's Guide: Writing Testable Code (while individual points are arguable the overall approach is something worth at least considering; can't post link, limited by reputation)

  • As also suggested ::broadcast() probably does more than it should, but I don't have enough understanding of your application to make a suggestion.

  • I would rename ::listeners() to ::getListeners(), for clarify and for consistency with ::getBroadcasts(). Same for other ::listeners##() methods. Somewhat nitpicky would be to suggest usage of null over an empty string to mark an empty value

  • Don't know if it's only for clarity here, but you keep referencing classes from the same namespace as Listeners using their fully qualified name. You can remove all those \module\core\ as it's all in the same namespace. You may also search for use keyword, may come in handy one day

As for your update:

  • You do not have to re-instantiate instances! If you want more parts of application (say classes) to have access to a certain instance you pass the instance to them in some way (like constructor or setter injection) and assign it to a property. (don't worry about passing-by-value as in case of instances PHP actually just passes an object ID, so it is pretty much a reference).

  • MD5 is not needed, PHP indexes are already being hashed. Some nice articles:

share|improve this answer

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.