Data Access Layer (object oriented)

Question

I am using a global class (like a global model) that acts like a sql abstraction layer for my application. I am using it without problems now, but since the application I am creating is getting bigger, I would like this class to be reviewed if this is really a fine abstrraction layer. I cannot use a proper ORM or framework at this time.

My models talk to the database. Therefore, they all inherit from global_model. I am using PHP's PDO and prepared statement facilities. I did not have functions for select queries because I found they can have different shapes and forms so I didn't implement them, I have no idea how. Therefore, all select statements, along with DDL statements ar executed directly in the subclasses. Here is my code.

spl_autoload_register(function($class_name) {
    include $class_name . '.php';
    });

abstract class global_model {
    protected $con;
    protected $table_name;
    protected $id; // the id most used for where clause and update statements
    protected $errors;
    protected $data; // inputs from a form. This is an array like $_POST. The fields from $_POST are first validated in the controller for correct formatted data before being passed here.

    public function __construct($data = null) { // null if not used for inserting or updating data
        $this->con = database::connect();
        $this->errors = array();
        $this->con->exec("use " . DB); // I am using different databases.
        if (isset($data)) {
            $this->data = $data;
        }
    }

    // $data is an array, usually fields gathered by $_POST after being validated for correct formatted data
    public function set_data($data) {
        $this->data = $data;
    }

    public function get_data() {
        return $this->data;
    }

    // if called by subclass. Use $this->data as the record to be inserted, and $this->table_name for the table to insert to.
    public function add() {
        return $this->add_external($this->table_name, $this->data);
    }

    // this one is an agnostic function if no appropriate models exist.
    public function add_external($table, $columns) {
        $names = "";
        $values = "";
        foreach ($columns as $column => $value) {
            $names .= "$column,";
            $values .= "?,";
        }
        $names = rtrim($names,",");
        $values = rtrim($values,",");
        try {
            $stmt = $this->con->prepare("insert into $table ($names) values($values)");
            return $stmt->execute(array_values($columns));
        } catch (Exception $e) {
            if ($this->con->inTransaction()) {
                throw $e;
            }
            $this->errors[] = "SQL failed: " . $e->getMessage();
            return false;
        }
    }

    // usually used in inserts with foreign keys
    public function get_current_id() {
        return $this->con->lastInsertId();
    }

    // if called by subclass. Use $this->data as the record to be edited, and $this->table_name for the table to edit to. The id use to edit is in $this->data['id'].
    public function edit() {
        return $this->edit_external($this->table_name, $this->data, $this->id); // $this->id is the name (not the value) of the columns used in the where clause for editing.
    }

    // this one is an agnostic function if no appropriate models exist. The argument $id_name is the name of the primary key column. For example, $id_name would be called user_id, and the value that will go into that column at this session is in $columns['id'].
    public function edit_external($table, $columns, $id_name) {
        // detach removes an item from an array and returns it
        // in this case, we separate the id (the value of the primary key of the table).
        $id = detach($columns, 'id');
        $names = "";
        foreach ($columns as $column => $value) {
            $names .= "$column=?,";
        }
        $names = rtrim($names,",");
        try {
            $stmt = $this->con->prepare("update $table set $names where $id_name=?");
            $values = array_values($columns);
            $values[] = $id;
            return $stmt->execute($values);
        } catch (Exception $e) {
            if ($this->con->inTransaction()) {
                throw $e;
            }
            $this->errors[] = "SQL failed: " . $e->getMessage();
            return false;
        }
    }

    // just a quick function to edit everything.
    public function edit_all($table, $columns) {
        $names = "";
        foreach ($columns as $column => $value) {
            $names .= "$column=?,";
        }
        $names = rtrim($names,",");
        $stmt = $this->con->prepare("update $table set $names");
        return $stmt->execute(array_values($columns));
    }

    public function get_errors() {
        return $this->errors;
    }

    // this just validates inputs from a form using a validator callback. I understand this function does not belong here. But I don't know where to put it and this function is used all the time while accessing the database.
    public function validate_input($element, $validator = null, $message = null) {
        $name = $element[0];
        $value = $element[1];
        if (!$validator($value)) {
            $this->errors[] = $message == null ? "Please input a valid " . ucfirst(str_replace("_", " ", $name)) . "." : $message;
            return false;
        }
        return true;
    }

    // I understand this function does not belong here. But I don't know where to put it and this function is used all the time while accessing the database.
    public function validate_phone_number(){
        return function($number) {
            return (is_numeric($number) && (strlen($number) == 7 || strlen($number) == 11));
        };
    }

    // I understand this function does not belong here. But I don't know where to put it and this function is used all the time while accessing the database.
    public function validate_email_address() {
        return function($var) {
            return filter_var($var, FILTER_VALIDATE_EMAIL);
        };
    }

    // $statements is a closure
    public function exec_transaction($statements) {
        try {
            $this->con->beginTransaction();
            $statements();
        } catch (Exception $e) {
            $this->con->rollback();
            // this will be change to log the errors in the future.
            $this->errors[] = "SQL failed: " . $e->getMessage();
            return false;
        }
        return true;
    }

    // I understand this absolutely does not belong here, but many times uploading images and inserting/updating needs to be an atomic operation.
    public function process_image($file_array) {
        return (isset($file_array["image"])) ? substr(upload_image($file_array, "image", "profile"), strlen(DIR)) : "images/default_picture.png";
    }

    // all business logic and validations go here. This will be called by the controller every time a database operation will be performed.

    abstract public function validate();

    public function __destruct() {
        database::disconnect();
    }
}

Here is the class that manages the connections.

class database {
    private static $con = null;
    private static $host = "localhost";
    private static $username = "root";
    private static $password = "";

    public static function connect() {
        if (!self::$con) {
            self::$con = new PDO("mysql:host=" . self::$host, self::$username, self::$password, array(PDO::ATTR_ERRMODE=>PDO::ERRMODE_EXCEPTION));
        }
        return self::$con;
    }

    public static function disconnect() {
        if (self::$con) {
            self::$con = null;
        }
    }
}

Although this works fine at this time, I believe my overall structure is extremely bad. How can I improve my code and include some other statements (particularly complicated selects and DDL statements)?

Answer 1

TLDR; version:

• This base model class is doing way too much.
• You are not doing a good job of guaranteeing your objects are set up in a valid state.
• Your singleton is not implemented properly.

More detailed thoughts:

include $class_name . '.php';

Should this be a require? I would think you would want your application to fail loudly if a class is unable to be loaded.

---

public function __construct($data = null) { // null if not used for inserting or updating data
    $this->con = database::connect();

Consider passing a valid database object in the constructor (dependency injection) vs. getting is statically. The problem with your static invocation here is that you are doing nothing to validate that you actually are returned a valid PDO object. You just assume that it is always going to work. If you did something like:

public function __construct(PDO $pdo, $data = null) { // null if not used for inserting or updating data
    $this->con = $pdo;

You would be 100% guaranteed from the perspective of this class that you have a valid dependency to work with, as you likely would not even trying to instantiate the class if the PDO instantiation failed. You are also type hinting the parameter as valid PDO object. The constructor would fail with exception if this condition is not met - a good thing as it saves you a lot of guarding code.

---

protected $table_name;

Consider making this a class constant. I am guessing there is no reason that an inheriting class (assuming one model class be DB table), would ever need to change this value, thus consider making it immutable.

---

public function set_data($data) {
    $this->data = $data;
}

No data validation here at all. You should strongly consider adding validation around all public methods where parameters being passed - either through type hinting, or inspection of parameter against expected data types/values. What is null is passed here? Is that a reasonable value? What if some other oddball data type (a PDO object for example) is accidentally passed to this method? You are allowing your object to be put into a potentially bad state as opposed to failing loudly (with InvalidArgumentException or similar)

---

// if called by subclass. Use $this->data as the record to be inserted, and    $this->table_name for the table to insert to.
public function add() {
    return $this->add_external($this->table_name, $this->data);
}

What value does this method bring?

---

// this one is an agnostic function if no appropriate models exist.
public function add_external($table, $columns) {
    $names = "";
    $values = "";
    foreach ($columns as $column => $value) {
        $names .= "$column,";
        $values .= "?,";
    }
    $names = rtrim($names,",");
    $values = rtrim($values,",");
    try {
        $stmt = $this->con->prepare("insert into $table ($names) values($values)");
        return $stmt->execute(array_values($columns));
    } catch (Exception $e) {
        if ($this->con->inTransaction()) {
            throw $e;
        }
        $this->errors[] = "SQL failed: " . $e->getMessage();
        return false;
    }
}

I think trying to implement a one query builder-fits all base method on your models is going to end up giving a TON of complexity and make you code hard to maintain. What columns are an play for a given concrete class become unclear. Why not simply make this abstract function that has to be implemented by all inheriting models? I know that this could lead to some extra code in your application, but I would rather have extra easy-to-read, easy-to-maintain lines of code in my app rather than something like this.

Same concern for edit_external().

---

public function get_current_id() {
    return $this->con->lastInsertId();
}

I am concerned with that fact that your model (which should represent a single instance of a given object in database) would have the ability to switch insert id's. Is the class a provider/factory or a model?

---

public function edit() {

What value does this method add?

---

public function edit_all($table, $columns) {

Why would a model that would seemingly be representing a single record have methods to operate against all records?

---

public function validate_input($element, $validator = null, $message = null) {

This should probably be abstract method to be defined in inheriting classes.

---

I don't understand why the validate_* methods are returning callbacks. Why are these in this class at all?

---

I think it is a significant problem to leave it up to the caller of the model to determine if an action which will change the data in the database needs to be done within a transaction or not. I would think that, based on the nature of the data, the model should enforce the methodology by which a recorded is changed. Why does this model class have the functionality to be passed arbitrary sets of statements for execution?

Also, where are you committing your transaction?

---

You are right that the process_image() method does not belong in this class.

---

Why should any one concrete model determine when the DB connection is discarded as is done in your __destruct() method? What if there are other models still active in the system that need this connection (you are trying to use a singleton after all. Again, just pass the PDO dependency to this class and leave the management of connections elsewhere in your code.

---

Your database class is wonky. It looks like you are trying to implement singleton pattern here, but it is not properly implemented. There is nothing here (i.e, private constructor or abstract class declaration) from preventing hundreds or thousands of copies of database objects from being created in code, each with their own DB connection. At that point you do not have a singleton.

If you truly want to use a singleton here (which I generally discourage as a pattern in favor of dependency injection), you need to prevent the class from being instantiated publicly, and you probably need to remove the disconnect() method otherwise there is nothing stopping objects using this class from setting up and tearing down connections all throughout your code. The typical concept of the singleton is to provide a single authoritative source for a dependency throughout the entire life cycle of the script

As it stands right now, this class adds no value vs. just direction instantiation of PDO objects.

Don't but DB credentials in your code. This is a horrible practice. These should ideally be derived from configuration.

Also some thoughts on variable naming in the class. database is generic and doesn't hold much meaning. How about something like PdoSingleton? Your connect() method doesn't really "connect" to the database (or at least it shouldn't other than the first time a PDO object is built_. It just provides a PDO instance, so maybe getPdoInstance() or similar might be more meaningful.

class PdoSingleton
{
    protected static $pdo;
    // connection settings derived from a constants perhaps set from config
    protected static $host = DB_HOST;
    protected static $username = DB_USER;
    protected static $password = DB_PASSWORD;

    private final function __construct() {
        // do nothing
    }

    public static function getPdoInstance() {
        if(empty(self::$pdo)) {
            try {
                return self::$pdo = new PDO(...);
            } catch (PDOException $e) {
                // maybe log error and rethrow
                // maybe don't try-catch here at all if you want exception to bubble up
            }
        }

        return self::$pdo;
    }
}