4
\$\begingroup\$

I'm trying to create a single class to interact with my database (MariaDB, in this case). Is there anything I can do to improve this code? I'm fairly new to Object-oriented PHP.

<?php
namespace TOB\Model;

use \TOB\Utility as U;

abstract class CRUDD {
    protected $table_name = "";
    public function __construct($data)
    {
        if (is_int($data)) {
            $this->read($data);
        } else if (is_string($data)) {
            $this->read($data, true);
        } else if (is_array($data)) {
            $this->create($data);
        } else {
            U\Error::report("Cannot determine course of action for '" . $this->table_name . "' in CRUDD.", 1);
        }
    }
    public function create($data)
    {
        /*
            Create an object and store it in the database.
        */
        $query = "INSERT INTO `" . $this->table_name . "` (";
        foreach ($data as $field => $value) {
            $fields[] = $field;
            $values[] = $value;
        }
        for ($i = 0; $i < count($fields); $i++) {
            if ($i == (count($fields) - 1)) {
                $query .= "`" . $fields[$i] . "`";
            } else {
                $query .= "`" . $fields[$i] . "`, ";
            }
        }
        $query .= ") VALUES (";
        for ($i = 0; $i < count($values); $i++) {
            if ($i == (count($values) - 1)) {
                $query .= "?";
            } else {
                $query .= "?, ";
            }
        }
        $query .= ");";
        $query = $mysql->handle->prepare($query);
        $this->read((int)$mysql->handle->lastInsertId());
        if ($query = $query->execute($values)) {
            return true;
        }
        return false;
    }
    public function read($data, $use_string = false)
    {
        /*
            Read an object from the database.
        */
        $mysql = new U\MySQL();
        if ($use_string) {
            $query_string = "SELECT * FROM `" . $this->table_name . "` WHERE `" . $this->table_name . "_link` = ? LIMIT 1;";
            if ($query = $mysql->handle->prepare($query_string)) {
                $query->bindParam(1, $data, \PDO::PARAM_STR, 32);
            } else {
                U\Error::report("Could not prepare statement '" . $query_string . "' in CRUDD.", 2);
            }
        } else {
            $query_string = "SELECT * FROM `" . $this->table_name . "` WHERE `" . $this->table_name . "_id` = ? LIMIT 1;";
            if ($query = $mysql->handle->prepare($query_string)) {
                $query->bindParam(1, $data, \PDO::PARAM_INT, 32);
            } else {
                U\Error::report("Could not prepare statement '" . $query_string . "' in CRUDD.", 2);
            }
        }
        $query->execute();
        if ($query->rowCount() >= 1) {
            foreach($query->fetch(\PDO::FETCH_ASSOC) as $key => $value) {
                if (substr($key, 0, (strlen($this->table_name) + 1)) == $this->table_name . "_") {
                    $key2 = substr($key, (strlen($this->table_name) + 1));
                    $this->$key2 = $value;
                } else {
                    $this->$key = $value;
                }
            }
        } else {
            foreach (get_object_vars($this) as $key => $value) {
                if ($key != "table_name") {
                    unset($this->$key); 
                }
            }
        }
    }
    public function update($field_name = null)
    {
        /*
            Update some or all of an object.
        */
        if (is_null($field_name)) {
            $mysql = new \Utility\MySQL();
            $query = "UPDATE `" . $this->table_name . "` SET ";
            foreach (get_object_vars($this) as $field => $value) {
                if (($field != "class_name") && ((substr($field, 0, strlen($this->table_name)) == $this->table_name) || ($field == "id"))) {
                    $fields[] = $field;
                    $values[] = $value;
                }
            }
            for ($i = 0; $i < count($fields); $i++) {
                if ($fields[$i] == "id") {
                    $fields[$i] = "" . $this->table_name . "_id";   
                }
                if ($i == (count($fields) - 1)) {
                    $query .= "`" . $fields[$i] . "` = ?";
                } else {
                    $query .= "`" . $fields[$i] . "` = ?, ";
                }
            }
            $query .= " WHERE `" . $this->table_name . "_id` = " . $this->id;
            $query = $mysql->handle->prepare($query);
            $query = $query->execute($values);
            $this->read($this->id, false);
        } else if (is_array($field_name)) {
            // Able to update a set of values only
        } else {
            // Able to update a single value
        }
    }
    public function delete()
    {
        /*
            Make an object unreadable by marking it as deleted. This is a clean delete.
        */
        $mysql = new \Utility\MySQL();
        $query = "UPDATE `" . $this->table_name . "` SET `" . $this->table_name . "_deleted` = 1 WHERE `" . $this->table_name . "_id` = " . $this->id . ";";
        $query = $mysql->handle->prepare($query);
        $query = $query->execute();
        $this->read($this->id, false);
    }
    public function destroy()
    {
        /*
            Remove an object from the database altogether. This is a 'messy' delete.
        */
        $mysql = new \Utility\MySQL();
        $query = "DELETE FROM `" . $this->table_name . "` WHERE `" . $this->table_name . "_id` = " . $this->id . ";";
        $query = $mysql->handle->prepare($query);
        $query = $query->execute();
        $this->read($this->id);
    }
}
\$\endgroup\$
3
  • 1
    \$\begingroup\$ There are errors in your code (eg the create method uses an undefined variable $mysql as though it were an object). Please note that this site only concerns actual, working code. Also look into dependency injection, and read this answer to learn why it applies to your code \$\endgroup\$ Commented Dec 23, 2014 at 20:37
  • \$\begingroup\$ Thanks, a very informative answer you linked to. How would I handle the PDO without using a MySQL class here. I've been warned against "global $pdo" in each function. \$\endgroup\$
    – Prinsig
    Commented Dec 24, 2014 at 8:42
  • \$\begingroup\$ Either pass a PDO connection to the constructor (public function __construct(PDO $db)), or create the connection in a service, and pass data models to that service, which acts as a primitive mapper/data provider. That's a lot cleaner in terms of separation of concern at least \$\endgroup\$ Commented Dec 27, 2014 at 15:10

1 Answer 1

1
\$\begingroup\$

Create method

Perhaps the create method should ensure $data is not empty - otherwise it could lead to SQL errors. Throwing an exception may be prudent.

Also in that method the loop to add field names could be simplified using the implode() function. If the first loop was updated so the field names were wrapped in backticks:

    foreach ($data as $field => $value) {
        $fields[] = "`$field`";
        $values[] = $value;
    }

Then the subsequent for loop:

   for ($i = 0; $i < count($fields); $i++) {
        if ($i == (count($fields) - 1)) {
            $query .= "`" . $fields[$i] . "`";
        } else {
            $query .= "`" . $fields[$i] . "`, ";
        }
    }

Can be replaced with this:

 $query .= implode(', ', $fields);

And similarly the list of placeholders can be created with array_fill()

$placeholders = array_fill(0, count($values), '?');
$query .= ") VALUES (" . implode(', ', $placeholders) . ");";

Comments

Most of the methods appear to have a multi-line comment within the method describing what the method does. A common convention in many Object-Oriented languages is to have such a comment appear just above the method definition. While it isn't finalized this is described in the draft for the PHP Standards Recommendation PSR-5 PHPDoc. The DocBlock can also provide information about parameters, return values, exceptions that may be thrown, etc. Many IDEs will index those docblocks and use them to offer suggestions during development.

Undefined Variable

As was mentioned in a comment, the variable $mysql does not appear to be defined in the create method:

$query = $mysql->handle->prepare($query);

Even if it was defined, $query gets assigned to the return value from calling the prepare method, so it starts as a string and then presumably contains a prepared statement. To avoid confusion for anyone reading the code (including your future self) it would be better to use a different variable for the prepared statement - for example- if there was a need to utilize the string representing the query.

Variable overwriting

At the end of that create method are these lines:

    if ($query = $query->execute($values)) {
        return true;
    }

It appears that $query gets re-assigned to the return value from the call to execute(). Again the variable gets overwritten, possible with a different type. This may lead to confusion if $query was used after these lines however there seems to be no point in assigning that value since $query is not used after that point.

Soft Deletion

It appears that the delete() method updates a deleted column, which is a technique for soft deletion. Should the read() method ensure that the value in the column is not 1?

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.