3
\$\begingroup\$

I'm currently creating a class that will have a bunch of my most common database functions. This is the first method I've made, I would appreciate any feedback, thanks!

public static function insert($table, $params) {
    // Define initial query
    $query = "INSERT INTO `{$table}` (";

    // Format rows to insert
    foreach($params as $key => $value) {
        $fields .= $key . ",";
        $bindParams .= ":" . $key . ",";
        $queryParams[':' . $key] = $value;
    }

    // Add rows and bind params to query
    $query .= rtrim($fields, ',') . ") VALUES(" . rtrim($bindParams, ',') . ")";

    // Prepare Query
    $preparedQuery = self::getInstance()->getConnection()->prepare($query);

    // Attempt to execute Query
    if(!$preparedQuery->execute($queryParams)) {
        return false;
    }

    // If everything passes, return true
    return true;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Does that code work? \$\endgroup\$ – Phrancis Dec 31 '16 at 20:09
  • \$\begingroup\$ @Phrancis Code works perfectly, just curious as to if there is a better way to create a custom insert method. \$\endgroup\$ – Joe Scotto Dec 31 '16 at 20:10
  • \$\begingroup\$ What's your PHP version? \$\endgroup\$ – Mast Dec 31 '16 at 20:17
  • \$\begingroup\$ @Mast Currently using 7.0.0. \$\endgroup\$ – Joe Scotto Dec 31 '16 at 20:17
4
\$\begingroup\$

The code, for what it is, looks fine. You're using prepared statements, which is a good thing, and the code comments help to understand what it does.

The real problem that I see is more of a design issue. Dynamically building SQL queries should be avoided unless it is absolutely necessary (such cases are pretty rare). They are error-prone, very difficult to debug, not practically testable, and their database execution performance is usually sub-par.

A better approach is to make a method/function for each kind of query you need. Granted, that does make it so you have to write a lot more code, but in the end it will make the code base a lot more maintainable, as well as testable, and your database server will thank you for it.

I would suggest reading The Codeless Code: Case 213 "The Imperfect Mirror" which illustrates how database query related code should be designed.

\$\endgroup\$
  • \$\begingroup\$ I'm just trying to prevent repeated code. I literally have 8+ "INSERT INTO" statements throughout my code. \$\endgroup\$ – Joe Scotto Dec 31 '16 at 20:27
  • 3
    \$\begingroup\$ Just write 8+ insert functions, you'll thank yourself later. \$\endgroup\$ – Phrancis Dec 31 '16 at 20:29
  • 1
    \$\begingroup\$ Just an update. I am now thanking myself for not creating a "one-fit" insert method. I now use a query method that includes the bind params and query in one call. \$\endgroup\$ – Joe Scotto Apr 2 '17 at 15:37
4
\$\begingroup\$

The challenge I have when I see people trying to build these sorts of general use database interaction classes, is that they most typically add limited value to the code base while more often then not doing things such as:

  • restricting the usage of the underlying database abstraction object (PDO in this case)
  • making the code harder to test and debug
  • convolute operations against an individual record vs. those against a collection of objects
  • splitting logic needed for making database updates across more classes than necessary - introducing heavy coupling along the way.

I would encourage you to really think about your class-level design.

What are you really trying to achieve? If your desire it to have various objects in your system have consistency in how they are persisted in the database, is this best met by enforcing a single insert method be used across all such objects?

Does it make sense to have a single method that can handle this logic for any object in your system that you could potentially think of, or is it better to enforce the behavior via composition using inheritance and/or interfaces?

What you seem to be striving for is what is commonly referred to as a database model - functionality that oftentimes is exposed though an Object Relational Mapper (ORM). ORM's are meant to provide the ability to map concrete objects in your system to records in a data store, and oftentimes provide methods for instantiating objects from data records and for working against collections of records.

I will present a class design which uses a sort of factory pattern to provide operations against collection of such models. These models inherit from a common base Model class.

I would like to highlight how this approach more properly separates your concerns.

Your concrete model implementations will hold ALL logic about what database table it needs to interact with, what fields the object has, how to validate appropriate values passed to class methods, etc. Now this class truly only represents one instance of whatever it is modeling. It does not need knowledge on how to instantiate a DB connection (one is passed to it by the factory), it does not need to know how to find other objects of its type in the data store (again this logic would reside in the factory), it doesn't need to know how to instantiate itself (the factory makes the instantiation). Your code in this class can now focus on the business logic of what it means to be an object of X class (what visible properties does it have? what actions can it perform?) as well as considerations for how data needs to be loaded (does the object lazy load from the data store or load all at once? Must updates be transactional between in memory object and data store?). This class owns how properties are put into the object (hydrated) from the DB or user input and own how updates are made. You can better enforce validation of values with this approach vs. your one-method-fits-all approach.

The factory will hold all knowledge on which class(es) it is a factory for, and how to operate against the COLLECTION of object data represented in the data store. It is also responsible for passing a DB connection when instantiating concrete Model objects. Again, we don't want this logic in the classes themselves, they need a PDO connection that "just works" so that you don't need to duplicate PDO instantiation error handling in all these classes.

You will note that with this approach, there is no more need to "wrap" a general use class around PDO. You have delegated behaviors to the appropriate entities to where you have eliminated much of the code duplication via inheritance and encapsulation. Yes, you will still need to implement some methods such as update() or insert() with a query specific to each class. But now that query would look something like:

INSERT INTO `user`
(name, email_address, field_x, ...)
VALUES
(:name, :email, :field_x, ...)

which means ALL logic for an insert (for say a user), is there in ONE method, plain as day, for a developer working on the application to easily understand exactly what fields are being worked with.

I would also like to note that you currently have no input validation whatsoever. This is important, especially around public methods. In your code, for example, you can get to the point of actually performing a query built from values that might be missing or malformed against a database connection that is being provided on the fly (without any validation). You can and should evaluate the data being passed to make sure it is safe to work with and not put your application in a bad state BEFORE you start working against. Fail fast and fail loudly (ideally by throwing exceptions). Don't continue to code only for the happy path or your will find your codebase extremely difficult to debug and maintain. Give preference towards passing the dependencies a piece of code requires to it in a proper state (an approach known loosely as "dependency injection"). For example, in the code I provide below, you will see how the PDO object gets passed to classes that need it while being enforced via type hinting in the methods. This sort of type hinting can save you a bunch of guarding code that would otherwise be needed to make sure you have a valid PDO object or similar.

It is EXTREMELY important in object-oriented programming to make sure concrete classes perform in an expected, uniform manner throughout the application, so you should ALWAYS write as much code is as needed make sure your object is set up in an appropriate state for it to interact with other code or throwing exceptions if it can't be set up in an expected manner.

Putting these concepts together, I present the following skeleton code example for your consideration:

// a base class for operating against collection of objects adhering to the model
abstract class PdoModelFactory
{
    // a property to store PDO object to be used globally for the factory
    protected static $pdo;

    // a property to store name of class that this factory should instantiate
    protected static $class;

    // a static function to inject PDO object to factory
    public static function setPDO(PDO $pdo) {
        self::$pdo = $pdo;
    }

    // a function that can be used within class to determine if factory has
    // been properly initialized. This can be overridden by inheriting classes.
    protected static function validateFactoryState() {
       if(self::$pdo instanceof PDO === false) {
           throw new Exception('Factory does not have PDO initialized');
       }
       $class = self::$class;
       if(empty($class::getTable())) {
           throw new Exception(
               'Class specified in factory does not have DB table defined.'
           ); 
       }
       if(empty($class::getPrimaryKeyField())) {
           throw new Exception(
               'Class specified in factory does not have primary key field defined.'
           ); 
       }          
    }

    // method to validate id value. Again overridable as needed.
    protected static function validateId($id) {
       $class = self::$class;
       // have class you are working with validate the input data
       try {
           $class::validateId($id);
       } catch Exception ($e) {
           // perhaps log an rethrow or wrap underlying exception
       }
    }

    protected static function validateFieldData(Array $data) {
        $class = self::$class;
        // have class you are working with validate the input data
        try {
            $result = $class::validateFieldData($data);
        } catch Exception ($e) {
            // perhaps log and rethrow or wrap underlying exception
        }
    }

    protected static function validateFieldExists($field) {
        $class = self::$class;
        // have class you are working with validate the input data
        try {
            $result = $class::validateFieldExists($field);
        } catch Exception ($e) {
            // perhaps log and rethrow or wrap underlying exception
        }
    }        

    // maybe provide some overridable functions for simplest of operations
    public static function getById($id) {
        self::validateFactoryState();
        self::validateId($id);

        // try to instantiate a new item of the class
        // passing PDO object to class as dependency
        try {
            $obj = new self::$class($id, self::$pdo);
        } catch Exception($e) {
            // perhaps log error and rethrow or recover
            $obj = null;
        }

        return $obj;
    }

    public static function getAll(PDO $pdo) {
        self::validateFactoryState();

        $collection = [];

        $sql = "SELECT `{self::$primaryKeyField}` FROM `{self::$table}`";

        // not shown - get list of id's and build array of objects in $collection

        return $collection;
    }

    public static function getAllPaginated($offset, $limit, $sortField, $sortOrder) {       
        self::validateFactoryState();
        self::validateFieldExists($sortField);

        $collection = [];

        // not shown build pagination query and build collection of objects

        return $collection;
    }


    public static function create(Array $data) {
        self::validateFactoryState();
        self::validateFieldData($data);

        // try to create a new instance of class
        try {
            $obj = new self::$class($data, self:$pdo);
        } catch Exception ($e) {
           // some handling here
        }
        return $obj;
    }

    public static function updateById($id, Array $data) {
        self::validateFactoryState();
        self::validateId($id);
        self::validateFieldData($data);

        // get instance of class from DB and update it.
        try {
            $obj = new self::$class($id, self::PDO);
            $obj->update($data);
        } catch Exception ($e) {
            // some handling
        }
        // perhaps return updated object
        return $obj;
    }

    // you might consider overriding this behavior to, for example,
    // rather than actually delete database record, instead update it to flag
    // as "deleted".
    public static function deleteById($id) {
        self::validateFactoryState();
        self::validateId($id);

        $sql = "DELETE FROM `{self::$table}`
                WHERE `{self::$primaryKeyField}` = :id";

        // not shown perform "delete" query returning true/false to caller
        // to indicate success
    }    

    // perhaps some abstract functions for operations against the collection
    // which really need class-specific knowledge
    abstract public static function findByValue($field, $value);
    ...  
}

class UserFactory inherits PdoModelFactory
{
    protected static $class = 'User';    

    // implement abstract methods
    public static function findByValue($field, $value) {
        self::validateFactoryState();

        $collection = [];

        // not shown - class-specific logic for allowing for field value searches

        return $collection;
    }
}

class Model implements JsonSerializable {
    // static properties to indicate behavior common across all class instances
    protected static $table;
    // properties to store primary key field name, or field configurations
    protected static $primaryKeyField = 'id';
    protected static $fieldConfig = [];

    // perhaps properties to store relationships to other models
    protected static $hasMany;
    protected static $belongsTo;

    // instance properties
    // these could represent fields you would expect on all models
    protected $id;
    protected $createdAt;
    protected $updatedAt;

    // this SHOULD be overridden in most inheriting classes to add additional logic
    // to hydrate the instance
    public function __construct($id, PDO $pdo) {
        self:validateId($id);

        $this->pdo = $pdo;
    }

    // common instance methods
    public function getId() {
         return $this->id;
    }
    // not shown - other common getters/setters as needed

    // overridable method to implement JsonSerializable
    // we just populate a StdClass object to be serialized
    // with only properties we want to expose.
    public function jsonSerialize() {
         $obj = new StdClass();
         $obj->id = $this->id;
         $obj->createdAt = $this->createdAt;
         $obj->updatedAt = $this->updatedAt;
         return $obj;
    }


    // abstract instance methods, where logic must live in inheriting classes
    // to execute desired behavior
    public abstract function update($data);
    public abstract function delete();

    // static methods
    // getter for table name
    public static function getTable() {
        return self::$table;
    }

    // method to validate id value. Overridable as needed.
    protected static function validateId($id) {
       $id = filter_var($id, FILTER_VALIDATE_INT)
       if($id === false || $id < 1) {
           throw new InvalidArgumentException(
               'Invalid Id value passed. Positive integer value expected.'
           );
       }
    }

    // abstract static methods
    // could be concrete methods if you find commonalities in your class logic
    protected abstract static function validateFieldData($data);
    protected abstract static function validateFieldExists($field);
}

class User extends Model {
    protected static $table = 'user';

    // here is where you would define fields for this concrete class
    protected static $fieldConfig = [
       ...
    ];

    // perhaps relate to other classes by class name
    protected static $hasMany = ['Group', 'EmailAddress'];
    protected static $belongsTo = ['Group'];

    // instance properties
    protected $name;
    ...

    public function __construct($id, $pdo) {
        parent::__construct($id, $pdo);

        // class specific logic which would include querying DB for record at $id
        // hydrating the class properties, etc.
    }

    // overriden jsonSerialize method
    public function jsonSerialize() {
        // start with parent property serialization
        $obj = parent::jsonSerialize();
        // add serializations specific to class
        $obj->name = $this->name;
        ...
        return $obj;
    }

    // implement abstract instance methods
    public function update(Array $data) {
        self::validateFieldData($data);

        // not shown some logic to update values in DB and locally upon success
        // perhaps update relationships if they have changed.
        // perhaps return success/failure flag
    }

    public function delete() {
        // perhaps perform DB deletion or deletion flag SQL
        // and then render the object unusable an capable of throwing exceptions
        // if ever accessed after this call.
    }

    // implement static abstract methods
    protected static function validateFieldData($data) {
        // not shown
    }

    protected static function validateFieldExists($field) {
        // not shown
    }
}

// Usage

// assume you have instantiated PDO connection into $pdo
// this could be in config file for example.
$pdo = new PDO(...);
UserFactory::setPdo($pdo);

// now later in code
// maybe user data has been input to create a new user record
$userData = ...;
$user = UserFactory::create($userData);
$user->update(['name' => 'new value');

// find by id
$idToFind = ...;
$user = UserFactory::getById($idToFind);

// serialize to JSON
$user_json = json_encode($user);

Update

I realized I didn't talk to interfaces at all to demonstrate how they might fit into the picture, so I have added an example.

Note how Model and User classes now implement JsonSerializable interface by adding new jsonSerialize() methods. This might be something you would commonly want to enable controlled serialization behavior over objects in your system. In this case, it allows you to determine how the object is represented if json_encode() is called upon it.

The usage example has also been updated.

\$\endgroup\$
  • \$\begingroup\$ This answer is wonderful, it should be canonical for code that is intended to work with databases. \$\endgroup\$ – Phrancis Jan 4 '17 at 2:23

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.