3
\$\begingroup\$

I am currently building a small application that allows users to create a slideshow, each slideshow consisting of slides that are either videos or images. Although this will be relatively small at this point, I want to build it using best practices and keeping in mind that this project will no doubt grow.

Before I get too far setting up my classes, I want to make sure my architecture is sound, as I am relatively inexperienced with objected oriented PHP but understand the importance of setting things up right in the first place.

Firstly - my PDO connection and class autoloading takes place in my config file, whilst also defining a few constants, one for development mode, one for ensuring direct access to included files is prevented. So, config.php...

<?php

define('DEVELOPMENT_MODE', true);

ini_set('error_reporting', E_ALL);
ini_set('display_errors', 1);

/** Used to prevent direct access to included files */
define('DIRECT_ACCESS', true);

/** Define DB Constants */
define('DB_HOST', 'localhost');
define('DB_NAME', 'dbname');
define('DB_USER', 'dbuser');
define('DB_PASS', 'dbpass');

/**
 * Connects to Database
 *
 * @return object PDO Instance
 */ 
function dbh_connect()
{
    try {
        $dbh = new PDO('mysql:host=' . DB_HOST . ';dbname=' . DB_NAME, DB_USER, DB_PASS);
        $dbh->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
    } catch (PDOException $e) {
        if (defined('DEVELOPMENT_MODE')) {
            echo $e->getMessage();
            exit;
        } else {
            echo 'Sorry, something seems to have gone wrong, we are aware of the problem and are looking into it. Please come back later';
            mail('myemail', 'Error Connecting to dbname Database', 'Error Connecting to dbname Database');
            exit;
        }
    }
    return $dbh;
}

spl_autoload_register(function($class)
{
    include 'mydirectory/classes/' . strtolower($class) . '.class.php';
});

I then have my first class, Slide class. I pass the db instance via dependency injection and set the private $dbh property to a PDO instance in my constructor.

I have also created a save method. It works, so no problems there. However, I am very conscious that classes should have one responsibility, and am wondering if this method should be somewhere else, in a different class. As you can see, the method could be copied and pasted into another class and work out of the box which is 1) good because it is portable, but 2) bad because this would be repeating myself.

<?php

class Slide
{
    private $id = null;
    private $ordernum = null;
    private $slide_type = null;
    private $location_id = null;
    private $status = null;

    private $dbh = null;

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

    public function __get($property)
    {
        if (property_exists($this, $property)) {
            return $this->$property;
        }
    }

    public function __set($property, $value) {
        if (property_exists($this, $property)) {
            $this->$property = $value;
        }
    }

    public function save()
    {
        try {
            $this->dbh->beginTransaction();
            $properties = array_filter(get_object_vars($this));
            unset($properties['dbh']);
            unset($properties['id']);
            $querystring = '';

            foreach ($properties as $prop => $value) {
                $querystring .= "{$prop} = :{$prop},";      
            }
            $querystring = substr($querystring, 0, -1);

            $stmt = $this->dbh->prepare("UPDATE slides SET " . $querystring . " WHERE id = :id");

            foreach ($properties as $prop => $value) {
                $stmt->bindValue(":{$prop}", $this->$prop);  
            }
            $stmt->bindValue(':id', $this->id);                
            $stmt->execute();

            $this->dbh->commit();
            return $stmt->rowCount(); // if row count is 0 need to return error, trigger error, exeption???

        } catch (PDOException $e) {

            $this->dbh->rollBack();

            if (defined('DEVELOPMENT_MODE')) {
                echo $e->getMessage();
                exit;
            } else {
                echo 'Sorry, something seems to have gone wrong, we are aware of the problem and are looking into it. Please come back later';
                mail('myemail', 'Error Connecting to dbname Database', 'Error Connecting to dbname Database');
                exit;
            }

        }
    }

}

Is a save method like this acceptable to have in a specific class like the Slide class? If not and it should be in some sort of DAO, how would that work exactly?

A method that looks up a slide in the database and then maps the columns to object properties via PDO::FETCH_CLASS, would this also reside in this slide class?

Note: I understand that the __get and __set magic methods are slower than custom getters and setters, but I do like how compact the code is. Is there a risk when running PDO::FETCH_CLASS that these methods would cause problems if my column names do not match the properties of the class?

\$\endgroup\$
1
\$\begingroup\$

The Slide class is mixing data access code and your Domain Model. These should be separated somehow, either by creating a parent class to encapsulate the "data access" or utilize the Repository Pattern to separate them.

  • Configuration is always messy. This is the one area where a static class with static methods makes sense. I would rewrite config.php into its own class, and remove any reference to PDO:

    ini_set('error_reporting', E_ALL);
    ini_set('display_errors', 1);
    
    // Used to prevent direct access to included files
    define('DIRECT_ACCESS', true);
    
    spl_autoload_register(function($class)
    {
        include 'mydirectory/classes/' . strtolower($class) . '.class.php';
    });
    
    class Config
    {
        private $data;
    
        private static function getData() {
            if (!isset($this->data)) {
                $this->data = parse_ini_file(__FILE__ . '../config.ini');
            }
    
            return $this->data;
        }
    
        public static function getDbUsername() {
            return $this->getData()['database']['username'];
        }
    
        public static function getDbPassword() {
            return $this->getData()['database']['password'];
        }
    
        public static function getDataSourceName() {
            return $this->getData()['database']['dsn'];
        }
    }
    

    Create an ini file to encapsulate your application config:

    environment = "development"
    
    [database]
    dsn           = "'mysql:host=localhost; dbname=mydb"
    username      = "..."
    password      = "..."
    

    This makes it easy to have multiple application environments for the same code base.

  • The Slide constructor takes an argument called $dbh. You can utilize PHP Type Declarations to communicate that an instance of PDO is required.

    class Slide
    {
        // ...
    
        public function __construct(PDO $dbh) {
            $this->dbh = $dbh;
        }
    }
    

    This helps reduce programming errors by communicating exactly what this class requires

  • Requiring a database connection object in a Domain Model is not correct. Either refactor this into a parent class or create another class responsible for the database CRUD operations on Slide objects.

    For instance, you could create a parent class for all your models:

    class Model
    {
        private static $dbh;
    
        protected function beginTransaction() {
            $this->getConnection()->beginTransaction();
        }
    
        protected function commit() {
            try {
                $this->getConnection()->commit();
            } catch (PDOException $e) {
                $this->rollBack();
                throw $e;
            }
        }
    
        protected function executeNonQuery(PDOStatement $statement) {
            $this->beginTransaction();
            $statement->execute();
        }
    
        private function getConnection() {
            if (!isset(Model::$dbh)) {
                Model::$dbh = new PDO(Config.getDataSourceName(),
                                      Config.getDbUsername(),
                                      Config.getDbPassword());
    
                Model::$dbh->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
            }
    
            return Model::$dbh;
        }
    
        protected function prepareStatement($sql) {
            return $this->getConnection()->prepare($sql);
        }
    
        protected function rollBack() {
            $this->getConnection()->rollBack();
        }
    }
    
    • The PDO connection object should be encapsulated in the parent class so sub classes must use protected methods on the parent class to run SQL against the database. This will reduce the chances for introducing bugs, and increase the maintainability of your application
    • The database connection object can be a static field on the class to promote sharing this object between model classes
    • All database transaction functionality should be in protected methods
    • The execution of PDOStatement's should be in a protected method.
    • No exceptions are swallowed! Please, please, please, please, please, please, please, please, please never ever ever ever ever ever catch exceptions and echo them or swallow them. If a database operation fails, let the exception blow things up sky-high. A higher layer of your application should handle the exception, log it, and redirect the user to some generic "Server Error" page.
  • Finally, your Slide class can extend from Model:

    class Slide extends Model
    {
        public function save() {
            $properties = array_filter(get_object_vars($this));
            unset($properties['dbh']);
            unset($properties['id']);
            $querystring = '';
            $params = array();
    
            foreach ($properties as $prop => $value) {
                $querystring .= "{$prop} = :{$prop},";      
            }
    
            $querystring = substr($querystring, 0, -1);
            $statement = $this->prepareStatement("UPDATE slides SET " . $querystring . " WHERE id = :id");
    
            foreach ($properties as $prop => $value) {
                $statement->bindValue(':' . $prop, $this->$prop);
            }
    
            $statement->bindValue(':id', $this->id);
    
            $this->executeNonQuery($statement);
            $this->commit();
        }
    }
    

If mixing your Domain Model with Data Access is not to your liking, then the Repository Pattern can help.

Decoupling Data Access From the Domain Model

The Repository Pattern allows you to completely decouple your Domain Model from the Data Access code. Instead of a class called Model we can refactor this into a common parent class for all your repository classes:

class BaseRepository
{
    private static $dbh;

    protected function beginTransaction() {
        $this->getConnection()->beginTransaction();
    }

    public function commit() {
        try {
            $this->getConnection()->commit();
        } catch (PDOException $e) {
            $this->rollBack();
            throw $e;
        }
    }

    protected function executeNonQuery(PDOStatement $statement) {
        $this->beginTransaction();
        $statement->execute();
    }

    private function getConnection() {
        if (!isset(BaseRepository::$dbh)) {
            BaseRepository::$dbh = new PDO(Config.getDataSourceName(),
                                  Config.getDbUsername(),
                                  Config.getDbPassword());

            BaseRepository::$dbh->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
        }

        return BaseRepository::$dbh;
    }

    protected function prepareStatement($sql) {
        return $this->getConnection()->prepare($sql);
    }

    public function rollBack() {
        $this->getConnection()->rollBack();
    }
}

Next, you'll want to create a SlideRepository class:

class SlideRepository extends BaseRepository
{
    private const TABLE_NAME = 'slides';

    public function find($id) {
        $statement = $this->prepareStatement("SELECT * FROM {SlideRepository::TABLE_NAME} WHERE id = :id");
        $statement->bindValue(':id', $id);
        $data = $statement->fetchAll();

        if (empty($data)) {
            return null;
        }

        return $this->map($data[0]);
    }

    public function save(Slide $slide) {
        $properties = array_filter(get_object_vars($slide));
        unset($properties['dbh']);
        unset($properties['id']);
        $querystring = '';
        $params = array();

        foreach ($properties as $prop => $value) {
            $querystring .= "{$prop} = :{$prop},";      
        }

        $querystring = substr($querystring, 0, -1);
        $statement = $this->prepareStatement("UPDATE {SlideRepository::TABLE_NAME} SET " . $querystring . " WHERE id = :id");

        foreach ($properties as $prop => $value) {
            $statement->bindValue(':' . $prop, $slide->$prop);
        }

        $statement->bindValue(':id', $slide->id);

        $this->executeNonQuery($statement);
    }

    private function map($row) {
        $slide = new Slide($row['id']);
        $slide->ordernum = $row['ordernum'];
        // Continue mapping columns to properties

        return $slide;
    }
}

This will remove all data access code from your Slide class, leaving it as a pure Domain Model that bundles data (in private fields) with behavior (in public methods).

Sample usage:

$repository = new SlideRepository();
$slide = $repository->find(23);
$slide->ordernum = 1;
$repository->save($slide);
$repository->commit();

A side effect of the Repository Pattern and exposing commit and rollBack methods publicly is you also implement the Unit of Work Pattern.

| improve this answer | |
\$\endgroup\$
3
\$\begingroup\$

Firstly, I'd just like to start by saying that I believe __get and __set magic methods are bad practice, and should be reserved for times when the alternative is either making member variables public, or when there is no logic in any things that are 'get' or 'set'. Even then, those are code smells of a "Data Class" which probably have a better solution. (Read "Refactoring" by Martin Fowler (1999)). Saying that, using it in a single class, within the top level abstract class for an Active Record or similar does make sense. Speed is rarely the issue these days; well-written code that is readable, flexible, and so on, is. But as detailed in a previous SO answers, use them sparingly and wisely.

I believe all your questions would be answered by referencing the above book, and "Design Patterns" (Erich Gamma et. al. 1995) and particularly for your question, "Patterns of Enterprise Application Architecture" by Martin Fowler (2003).

Fowler (2003) goes into various OOP patterns for data layers. It will give you some real epiphanic moments, and you'll never let the book leave your side.

To give you some insight as to why the book is so important to read, the decision about how to implement your data layer depends on the rest of your system architecture. It's not a case of one layer being dependent on the other, since that is an anti-pattern, but rather how correctly chosen layers can have a synergy (while still being as independent as possible).

There are different data source patterns he covers:

  • Table data gateway
  • Row data gateway
  • Active record
  • Data mapper

And then more complex Object-relational mapping, structural and metadata patterns.

I believe a wise nugget of wisdom I've read in many programming books are to not over-complicate things. It's not an easy balance, but if you use techniques like writing tests as you write code, (TDD or milder versions) then you will not be afraid of refactoring to a more complex pattern when you know you will use it. The ultimate goal really is to make your life as easy as possible.

The most concrete advice I can give you is keep your code DRY. So, don't copy and paste code. Ever. Just about anything is better than that.

And to answer your question, out of the patterns in Fowler (2003), I would suggest the Active Record pattern. It sort of gives a domain-level interface for accessing data in an intuitive way that can be a very tidy solution for smaller applications. (In my experience, one with no complicated joins, although there are exceptions.) You might implement it like this:

Write an abstract class that understands how to access the database and query it. Then for each table (or even a set of tables, such as a person and related tables like the person's phone numbers) write a subclass that uses the services of the parent, and define a top level interface which would be used by your domain code. For example, defining $person->setPhoneNumber($number). It really bundles up the data layer tidily.

I can't recommend the book strongly enough - it gives advice even on how to mix and match related patterns, showing that they aren't actually mutually exclusive - sometimes it makes sense to have different patterns for the same layer, depending on the problem in that portion of the application.

But to give you some confidence, it's worth remembering that patterns are just that; they're not identical implementations each time; the developer often will tweak things. There are some general principles to follow, such as S.O.L.I.D., and just following your nose. If something smells bad, or if things just seem to be getting messy, take a step back and investigate alternatives.

| improve this answer | |
\$\endgroup\$

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.