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);
}
}
1 Answer 1
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?
createmethod uses an undefined variable$mysqlas 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\$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\$