views:

508

answers:

13

Hi Guys,

I have a chicken-egg problem. I would like too implement a system in PHP in a OOP way, in which two classes would play important roles: Database and Log. My idea was to build up the connection by the Database class, which would have public methods eg. runQuery(sUpdateQuery), doInsert(sInsert), etc. The Log class would writing logs through common methods just as like logMessage(message), logDatabaseQuery(sQuery) TO THE DATABASE. The problem comes now.

1: Inside the methods of the Database's class I would like to be able to use the Log class's logDatabaseQuery(sQuery)

2: This still would not be a big challenge if I would not like to use the Database class's doInsert(sInsert) method inside the logDatabaseQuery method.

I would like to keep it simple - and just use one instance of the database connection object, and from the loggeras well, if it is possible.

For many people Singleton model would be the first idea to choose, but I definitely would like to use different solution.

So there would be two classes which would use each-other's methods:

Database doInsert logDatabaseQuery

Log logDatabaseQuery doInsert

I would like to keep the Log methods separately (in the Log class), since later on it would have other methods for logging not just to the database, but to files or e-mails as well.

Any ideas, how this should / could be done in the nicest, OOP friendly way?

I was thinking about a common parent abstract class, or about using interfaces as well, but finally could not figure out the proper way :(

What I would like to know is a suggestion for a correct class hierarchy

A: 

Create an insert method overload that allows for inserts without logging, and have your logging class use that. Otherwise, your design is recursive by definition: Log all DB inserts by performing a DB insert.

AaronSieb
A: 

add an optional parameter to doInsert $callerIsLogger=false, then everytime you need to doInsert() from your logger, provide the second parameter true, and your doInsert could check for this situation and not call the logger when is called by logger. What hierarchies? KISS methodology rocks :)

Peter Perháč
A: 

This is a good candidate for the Mediator Pattern. You would have an object that the logger and database would both invoke various methods on, and this mediator object would keep references to the logger and database and handle communication for you.

rlbond
A: 

Create a Logger class that implements a interface ILogger. A new instance of the Logger class receives a Object that implements the interface ILoggingProvider that is used to output log messages.

Create a Database class that implements the ILoggingProvider interface. A new instance of the Database receives a object that implements the ILogger interface that is used to log messages.

public interface ILogger
{
   void Debug(String message)
}

public interface ILoggingProvider
{
   void Log(String message)
}

public class Logger : ILogger
{
   private ILoggingProvider LoggingProvider { get; set; }

   public Logger(ILoggingProvider loggingProvider)
   {
      this.LoggingProvider = loggingProvider;
   }

   public void Debug(String message)
   {
      this.LoggingProvider.Log(message);
   }
}

public class Database : ILoggingProvider
{
   private ILogger Logger { get; set; }

   public Database(ILogger logger)
   {
      this.Logger = logger;
   }

   public void DoStuffWithTheDatabase()
   {
      // Do stuff with the database
      this.Logger.Debug("Did stuff with the database.");
   }

   public void Log(String message)
   {
      // Store message to database - be carefull not to generate new
      // log messages here; you can only use the subset of the Database
      // methods that do not themselve generate log messages
   }
}
Daniel Brückner
Doesn't that leave you with a recursion problem?
Mark Ransom
That is exactly what S.Lott pointed out. You cannot use logging everywhere in the Database class - so it would be good to explicitly seperate these parts.
Daniel Brückner
+4  A: 

It sounds like you have two different database accesses - logged (normal case) and unlogged (for the logging functions). Split the database class into two, a higher-level version for logged requests, and a lower-level version for unlogged requests. Implement the higher level database class using references to the logging and lower level database classes.

Mark Ransom
You can write the database class so that it may log activities if a logger object is available, and so that it won't log activities if a logger object isn't available. Writing two database classes isn't necessary.
Frank Crook
Sure, you COULD do it that way. But the question asked for an object-oriented solution that reused one instance of a database object, and I think this is the cleanest way to do so.
Mark Ransom
A: 

You could add another class that both the Database and Logger use to avoid getting into an infinite logging loop (and avoid the circular dependency as well).

// Used only by Logger and DatabaseProxy
class Database {
   function doInsert( $sql ) {
      // Do the actual insert.
   }
}

class Logger {
   function log($sql) {
     $msg_sql = ...
     Database.doInsert($msg_sql);
}

// Used by all classes
class DatabaseProxy {
   function doInsert( $sql ) {
      Logger.log($sql);
      Database.doInsert($sql);
   }
}

You could dress it up a bit by having both Database and DatabaseProxy implement a common interface as well, and use a factory to provide the appropriate instance.

Eric Petroelje
+5  A: 

You have combined too many things together.

The database can't really depend on a logger which depends on the database. It isn't good design.

What you really have are two kinds of database access.

Low-level access does "raw" SQL.

The logger can depend on this lower-level class. It doesn't -- itself -- have raw SQL in it. It depends on a lower-level class.

High-level access does application queries, it uses low-level access and the logger.

S.Lott
Putting "raw" SQL in your logger class is a great way to make a mess you'll have to clean up later. Just pass it to a database object and pass the database object to it.
Frank Crook
there's no requirement that the lower-level DB do only raw SQL. if you want, make an extra layer: 1:raw SQL, 2:OOP DB (uses 1), logger (uses 2), 3: logged DB (uses 2 and logger)
Javier
+3  A: 

If you want to give the database object access to a logger, then you pass the logger to it. If you want to give your logger access to a database object, then you pass the database object to it.

Check to see if those objects exist within the class before you use functions that are provided by them.

Since PHP passes these objects by reference by default, you can do this with a single logger and database class for several objects.

I'd also recommend you do all of your database writing at a single point in the logger. Just store everything in a temporary array and run your DB code when you deconstruct. Otherwise, you'll create a ton of overhead if you're writing to the database throughout your code.

class Database {
    private $_logger = 0; //contains your logger
    /* The rest of your class goes here */

    //Add the logger to your class so it may access it.
    public function setLogger($logger) {
        $this->_logger = $logger;
    }

    //To check if we have a logger, we don't want to use one if we lack one.
    public function hasLogger() { 
        if($this->_logger) return true; 
        else return false;
    }
}
class Logger {
    private $_database = 0; //variable to hold your database object
    /* The rest of your class goes here */

    public function setDatabase($database) { //Same crap basically
        $this->_database = $database;
    }

    public function hasDatabase() {
        if($this->_database) return true;
        else return false;
    }

    public function doSomething { //This is new though
        if($this->hasDatabase()) { //Shows how you use hasDatabase()
            $this->_database->someDatabaseFunction();
        }
        else {
            //Whatever you'd do without a database connection
        }
    }
}
$database = new Database;
$logger = new Logger;

$database->setLogger($logger);
$logger->setDatabase($database);
Frank Crook
A: 

IMHO, the database class should not be interacting with the logger. I would be inclined to call the logger from the same part of the code that is calling the database class. Then the logger can use the database class to do its inserts.

Scott
+2  A: 

Decouple Database from Logger. I would design the application to log from the middle tier and make the decision about which logger type to use at runtime not compile time. I.e. use a factory method to determine whether to log to db/xml/whatever.

If the Data Layer does need to log (i.e. report a problem) have it throw an exception, catch it in the middle tier and then decide how to handle it there or hand it off to a diagnostics class and have that decide. Either way I'd keep the DAL as "blind/dumb" as possible and not have it making decisions about what is and what is not a loggable event.

A common parent class is not a good idea. A logger is not a database[r]. And nor is a database a logger. Its not so much a chicken and egg problem as it is a cow and a pig problem. They are two different animals. There is no reason for Database to be aware of logger. I think you are forcing the abstraction. All i see is that a Logger has a Database.... if it's a db logger that is.

You drive the data layer from the middle tier anyway, so when including exceptions and events, I cant see where you would lose fidelity in logging db related events.

 public interface ILoggingProvider
    {
       void Log(String message)
    }

    public static class Logger
    {
       public static ILoggingProvider GetLoggingProvider()
       {
          //factory to return your logger type
       }

       public void Log(String message)
       {
          Logger.GetLoggingProvider().Log(message);
       }
    }

    public class DBLogger : ILoggingProvider {

      void Log(string message) {
       Database db = new Database();
       db.doInsert(someLoggingProc);
     }
    }

    public class Database
    {
        runQuery(sUpdateQuery) 
        doInsert(sInsert)
    }

...


 public class Customer{

 public void SaveCustomer(Customer c)
 {
   try {
    // Build your query
    Database db = new Database();
    db.runQuery(theQuery);
   } catch (SqlException ex) {
    Logger.Log(ex.message);
   }
 }
}
rism
A: 

sounds like you adding a lot of complication to a simple problem purely for the sake of making it object oriented...

ask yourself this, what are you really gaining by taking that approach?

as much as i hate "OOP for the sake of OOP" (or any design style 'just because'), in this specific case, it's not a bad design, and well done can be much more maintainable than non-OOP sequential code (is there a name for that? no, 'imperative' is not)
Javier
'Procedural', I think is the word
drhorrible
i disagree, he needs a single interface module for a single database and a single log module. any time the term singleton comes into play you've admitted that an OOP approach has failed to fit the problem. if he had several log type modules or several database back ends, then yea OOP all the way
A: 

You could make a second arg for your Database::insert() function:

function insert($sql, $logThis = true) { ... }

And then obviously when the logger calls it, make the second argument false.

Alternatively, just check the function stack to see if the insert function was called from the Logging class.

function insert($sql) {
    $callStack = debug_backtrace();
    if (!isset($callStack[1]) || $callStack[1]['class'] !== "Logger") {
        Logger::logSQL($sql);
    }
    // ...
}
nickf
A: 

How I would do it:

public interface ILogger
{
    void LogWarning(string message);
    void LogMessage(string message);
}

public interface ILoggingProvider
{
    void LogEntry(string type, string message);
}

public interface IDbProvider
{
    void DoInsert(string insertQuery, params object[] parameters);
}

public class Logger: ILogger
{
    private ILoggingProvider _provider = ProviderFactory.GetLogProvider();

    public void LogWarning(string message)
    {
        _provider.LogEntry("warn", message);
    }

    public void LogMessage(string message)
    {
        _provider.LogEntry("message", message);
    }

    public void LogEntry(string type, string message)
    {
        _provider.LogEntry(type, message);
    }
}

public class Database : IDbProvider
{
    private Logger _logger = new Logger();
    public void DoInsert(string insertQuery, params object [] parameters)
    {
        _logger.LogEntry("query", insertQuery);
    }
}

public class DbLogProvider : ILoggingProvider
{
    private IDbProvider _dbProvider = ProviderFactory.GetDbProvider();

    public void LogEntry(string type, string message)
    {
        _dbProvider.DoInsert("insert into log(type,message) select @type,@message",type,message);
    }

}

public static class ProviderFactory
{
    public static IDbProvider GetDbProvider()
    {
        return new Database();
    }


    internal static ILoggingProvider GetLogProvider()
    {
        return new DbLogProvider();
    }
}

Although I would probably also make it look up a type from a config file rather than hardcoding them in the ProviderFactory class. The assumption here is that your code doesn't care how it is logged, that's up the administrator, and that all logging would be done one way during execution(which would my preference anyway). Obviously you could extend this to make a choice as to the log target when creating the logger and create the appropriate provider.

Darren Clark
Same recursion problem. I almost answered a comment above. Yes, you need to separate into a non-logging DbProvider that just does the database access.
Darren Clark