2
\$\begingroup\$

This is my first "large scale" system I've designed for myself. Since I'm more of a front end developer than backend developer, I'm not sure if my architecture is sound.

I've created a Factory class which I use to create all my classes and connect to the DB. I think I've tried to use used Singelton(?) pattern, but I'm not really sure.

It probably would be easier if you knew more about the system, but in short it's an online directory mixed with user interaction and social media.

It would be great if I could get some feedback on this.

My factory class

include_once('abc.class.php');

class Factory {
  // One function for each class I include
  // This is the only place I include class files
  function new_abc_obj($id = NULL) { return new Abc(Conn::get_conn(), $id); }  
}

My DB connection class

class Conn {
  private static $conn = NULL;

  /* Establish PDO DB connection */
  private static function init() {
      $conf = self::config();
      try { 
        self::$conn = new PDO($conf['dsn'], $conf['user'], $conf['pass'], array(PDO::MYSQL_ATTR_INIT_COMMAND => "SET NAMES utf8"));
      } 
      catch (PDOException $e) {
        // We remove the username if we get [1045] Access denied
        if (preg_match("/\b1045\b/i", $e->getMessage())) 
          echo "SQLSTATE[28000] [1045] Access denied for user 'name removed' @ 'localhost' (using password: YES)";
        else
          echo $e->getMessage();  
      }
  }

  /* Get DB connection */
  public static function get_conn() {
    if (!self::$conn) { self::init(); }
    return self::$conn;
  }

  /* Get username, password and db data from config.php */
  private static function config() {
    $conf = array();

    $conf['user']    = DB_USER;
    $conf['pass']    = DB_PASSWORD;
    $conf['dsn']     = 'mysql:dbname='.DB_NAME.';host='.DB_HOST;

    return $conf;
  }  
}

My class interface

interface iAbc {
  public function one();
  public function two();
}

My Business Logic Layer (BLL)

class Abc extends AbcDAO implements iAbc {
  private $data = array();
  protected $db;

  function __construct(&$db, $id) {
    $this->db = &$db;

    if($id) {
      $this->setData($id);
    }
  }

  // __set, __get, __isset is here

  public function one(){};
  public function two(){
    $result = parent::get_some_data();
    return $result;
  };
}

My Data Access Layer (DLL)

class AbcDAO {
  protected function get_some_data($name){
    $sql ="SELECT 123;";
    $stmt = $this->db->prepare($sql);
    $stmt->bindParam(':name', $name, PDO::PARAM_STR);
    $stmt->execute();
    $result = $stmt->fetchAll();
    return $result;
  }
}

And this is how I create the object:

$my_abc = Factory::new_abc_obj();
$test = $my_abc->two();
\$\endgroup\$
3
\$\begingroup\$

Factory

Three things about this:

  1. Consider using an autoloader (see PRS-0). Then you don't have to bother about, where files get included.
  2. A Factory is used to create a set of objects, which depends on some context. For example, you could have a Filter and a Renderer for XHTML or HTML5. In order not having to check which one is needed again and again, you have an XhmtlFactory and an Html5Factory with createFilter() and createRenderer() methods.
  3. Factory methods usually are named create....

In your context, a Factory is not helping you.

DB Connection

Don't use static methods, if you can avoid it. Inject dependencies whereever possible. Do not (finally) handle errors within your classes. Leave that up to the caller. The method to get an instance is usually called getInstance() (and is one of the few cases, where static calls are reasonable). The initialization is then made in a private constructor.

class Conn
{
    private $conn = null;

    /**
     * Establish PDO DB connection
     */
    private function __construct(Config $conf)
    {
        try { 
            self::$conn = new PDO($conf->dsn, $conf->user, $conf->pass, array(PDO::MYSQL_ATTR_INIT_COMMAND => "SET NAMES utf8"));
        } catch (PDOException $e) {
            $message = $e->getMessage();
            // We remove the username if we get [1045] Access denied
            if (preg_match('/\b1045\b/i', $message)) {
                $message = "SQLSTATE[28000] [1045] Access denied for user 'name removed' @ 'localhost' (using password: YES)";
            }
            throw new ErrorException($message);
        }
    }

    /**
     * Get DB connection
     */
    public static function getInstance(Config $conf)
    {
        if (is_null($this->$conn)) {
            $this->conn = new Conn($config);
        }
        return $this->conn;
    }
}

class Config
{
    public $dsn  = 'mysql:dbname=' . DB_NAME . ';host=' . DB_HOST;
    public $user = DB_USER;
    public $pass = DB_PASSWORD;
}  

To make the singleton pattern in Conn complete, the magic method __clone() should made private, so the connection cannot be copied.

Business Logic Layer

Keep the business and data access layer separated. The business layer ideally does not know anything about storage. The data access layer is injected into the business layer.

class Abc implements iAbc
{
    private $data = array();
    protected $dal;

    public function __construct($dal, $id = null)
    {
        $this->dal = $dal;

        if (!is_null($id)) {
            $this->setData($id);
        }
    }

    // __set, __get, __isset is here

    public function one(){};

    public function two()
    {
        $name = 'foo';
        $result = $this->dal->get_some_data($name);
        return $result;
    };
}

Data Access Layer

class AbcDao
{
    private $db = null;

    public function __construct(Conn $db)
    {
        $this->db = $db;
    }

    public function get_some_data($name)
    {
        $sql ="SELECT 123;";
        $stmt = $this->db->prepare($sql);
        $stmt->bindParam(':name', $name, PDO::PARAM_STR);
        $stmt->execute();

        $result = $stmt->fetchAll();
        return $result;
    }
}

Controller

In your controller, you now really have control.

$config = new Config;        // You could use any other set of config data
$conn   = new Conn($config); // You could use any other storage method
$my_abc = new Abc($conn);
$test   = $my_abc->two();

or, shorter

$my_abc = new Abc(new Conn(new Config));
$test   = $my_abc->two();
\$\endgroup\$
4
  • \$\begingroup\$ Awesome answer! It's a bit sad that I'm the first upvoter. \$\endgroup\$ – Quentin Pradet May 13 '13 at 8:25
  • \$\begingroup\$ @nibra Thanks for in depth answer. One of the reasons why I created the factory was so that I don't have to call the DB connection every time I create a class. Only create the DB connection once, and use it for all the classes. Is this possible? Why can't new Config be inside Conn()? Do you know if this autoloader will conflict with he autoloader Wordpress is using? \$\endgroup\$ – Steven May 15 '13 at 17:57
  • \$\begingroup\$ DB Connection: You create it once and inject it into other objects for use. Config: If new Config is inside con, you loose the possibility to use another configuration (eg. for testing). \$\endgroup\$ – nibra May 15 '13 at 21:29
  • \$\begingroup\$ The autoloader question could be good as new question. That answer does not belong here. \$\endgroup\$ – nibra May 15 '13 at 21:31

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.