current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 2 down vote favorite

I have two classes, Database and Logger. Database is responsible for handling SQL queries, and Logger logs messages to an external file. Whenever the database executes an SQL query, I want Logger to log it. Right now my implementation looks something like this:

    class Database {
        public function query($sql) {
            Logger::log($sql); 

            // ...
        }
    }

    class Logger {
        public static function log($msg) {
            // ...
        }
    }

But I feel that this is bad practice because Database shouldn't know about Logger. So instead I was thinking of creating a publisher-subscriber model (or something similar) where Database keeps a list of subscribers and has a method to subscribe to events, then it alerts subscribers when an event is fired (eg: when query is executed). I'm thinking something like this:

    class Database {
        $subscribers = array(); 

        public function subscribe($something) {
            $subscribers.push($something);
        }

        private function alert_subscribers($msg, $event_type) {
            foreach ($s as $subscribers) { 
                // Alert all subscribers, somehow
            }
        }

        public function query($sql) {
            alert_subscribers($sql, SQL_QUERY_EXECUTED);

            // ...
        }
    }

    class Logger {
        public static function log($msg) {
            // ...
        }
    }

I want it to have multiple event types, so basically different groups are alerted when different events happen. I also don't know how the Logger should subscribe to Database under this design.

How should I go about implementing this? I'm not sure how to start.

share|improve this question
up vote 3 down vote accepted

There is nothing wrong with having the loggers everywhere. If you clutter all your classes with some kind of event notification it would look worst :)

But, Logger should not be called with a static method. You can create a pool of Loggers and your call will look like:

$log=Logger::get("Database");
$log->info("foo");

The big advantage is that you can put a configuration file in your project and define the log level of every logger independently.

According your questions you have really big plans. You want to write a template engine, a logging framework, an ORM and I guess somewhere in between your real application. Trust me when I tell you, you will fail. (I and most likely everybody else here tried it already :) ). For all your components there are mature frameworks out there. Find the one you like!!

share|improve this answer
1  
For what it's worth. I think you need to atleast try it once. If you do this, you will learn a lot on how things are build up. – Vince V. Feb 13 '13 at 11:29
1  
+1 But you should do it in your spare time not in a project you have to life with the next X years ;) – mnhg Feb 13 '13 at 12:00
    
That's true! :) – Vince V. Feb 13 '13 at 12:16
up vote 2 down vote

Event

class Event {

    private $_subscribers = array();

    public function Invoke() {
        foreach ($this->_subscribers as $subscriber) {
            call_user_func($subscriber);
            //call_user_func_array($subscriber, array("some argument"));
        }
    }

    public function Subscribe($subscriber) {
        if (!is_callable($subscriber)) {
            throw new \ArgumentException("Subscriber have to be callable");
        }

        $this->_subscribers[] = $subscriber;
    }

    public function UnSubscribe($unsubscriber) {
        foreach ($this->_subscribers as $key => $subscriber) {
            if ($subscriber == $unsubscriber) {
                unset($this->_subscribers[$key]);
                return true;
            }
        }

        return false;
    }

    public final function __invoke() {
        $this->Invoke();
    }

}

I would create at least a simple Event class to keep clean the code. The example above is a very simple one but if you have 5 more minutes for this you can create a DatabaseEvent class which have a Subscribe method what is expecting an IDatabaseEventHandler "interface instance" as parameter (public function Subscribe(IDatabaseEventHandler $handler)) which has a few or a lot of methods: LogConnectionError($arg), LogQueryError(QueryErrorArgument $arg) and so on. Whith this approach you will need an adapter class as a default interface implementation of the IDatabaseEventHandler for easier usage (for example when you are writing your unit tests). Of course the DatabaseEvent class have to have a lot of named log methods to call the subscribed handlers when it's neccessary.

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.