3
\$\begingroup\$

I'm learning PHP and currently object-oriented programming. I would like to get a feedback; how I'm doing so far. If I have bad coding that should be fixed immediately etc. I'd call this code an improved version. Because people said that my insert, update and delete methods were redundant. Do you think they're not redundant now?

This is the database class I've created after discovering that you can create a database class on top of the PDO class.

Connection Class:

<?php

// DBConnect.php

/**
* DBConnect class
*
* Database abstraction layer
*
* @file class-dbconnect.php
*
* @author Joey
*
* @version Release: 1.0
*
* @copyright 2014
*/
class DbConnect
{

    /**
    *
    * @var string
    * @access private
    *
    * Hold the database connection details
    */
    private $host;
    private $username;
    private $password;
    private $dbname;
    private $con;

    /**
    *
    * Used to hold the results from a query
    * 
    * @var object
    *
    * @access private
    */
    private $stmt;

    /**
    *
    * Rows from a query
    *
    * @var array
    *
    * @access private
    */
    private $row;
    private $rows;

    /**
    *
    * Row count
    * @var int
    *
    * @access private
    */
    private $row_count;

    /**
    *
    * Database status
    *
    * @var bool
    *
    * @access private
    */
    private $is_connected;

    /**
    *
    * Get the database details
    *
    * @param string
    */
    public function __construct($host, $dbname, $username, $password)
    {

        $this->host = $this->clean_param($host);
        $this->dbname = $this->clean_param($dbname);
        $this->username = $this->clean_param($username);
        $this->password = $this->clean_param($password);

        $this->is_connected = true;

        $dsn = 'mysql:host=' . $this->host . ';dbname=' . $this->dbname;
        try {
            $this->con = new PDO($dsn, $this->username, $this->password);
            $this->con->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
            $this->con->setAttribute(PDO::ATTR_DEFAULT_FETCH_MODE, PDO::FETCH_ASSOC);
        } catch (PDOException $e) {
            echo 'Error connecting to the database! Check the log file for more information.';
            $this->is_connected = false;
            $this->log_err($e->getMessage());
            exit;
        }
    }

    /**
    *
    * Close the database connection
    */
    public function close_connection()
    {
        $this->con = null;
        $this->is_connected = false;
    }

    /**
    *
    * Function for inserting data
    *
    * @param string
    */
    public function insert($sql, array $data)
    {
        try {
            $this->stmt = $this->con->prepare($sql);
            $this->stmt->execute($data);
        } catch (PDO $e) {
            echo 'Error inserting data.', '<br>';
            $this->log_err($e->getMessage());
        }
    }

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

    public function update($sql, array $data)
    {
        return $this->insert($sql, $data);
    }

    public function delete($sql, array $data)
    {
        return $this->insert($sql, $data);
    }

    /**
    *
    * Fetching a single row
    *
    * @param string
    *
    */
    public function get_row($sql, array $data)
    {
        $this->insert($sql, $data);
        return $this->stmt->fetch();
    }

    /**
    *
    * Fetching multiple rows
    *
    * @param string
    *
    */
    public function get_rows($sql, $data = array())
    {
        $this->insert($sql, $data);
        return $this->stmt->fetchAll();
    }

    /**
    *
    * Getting row count
    *
    * @return int
    */
    public function get_row_count()
    {
        $this->row_count = $this->stmt->rowCount();
        return $this->row_count;
    }

    /**
    *
    * Returns true if row count is greater than 0 Returns false otherwise
    *
    * @access public
    */
    public function check_row_count()
    {
        if ($this->get_row_count() > 0) {
            return true;
        } else {
            return false;
        }
    }

    public function query($sql, $data = array())
    {
        $this->stmt = $this->con->query($sql);

        if ($this->check_row_count()) {
            if ($this->get_row_count() > 1) {
                return $this->stmt->fetchAll();
            } else {
                return $this->stmt->fetch();
            }
        } else {
            echo 'Error in your query.', '<br>';
        }
    }

    /**
    *
    * Used to truncate a table
    */
    public function fresh_table($table)
    {
        $sql = 'TRUNCATE ' . $table;

        try {
            $this->stmt = $this->con->prepare($sql);
            $this->stmt->execute();
        } catch (PDOException $e) {
            echo 'Error truncating your table.', '<br>';
            $this->log_err();
        }
    }

    public function check_database_connection()
    {
        if ($this->is_connected) {
            return true;
        } else {
            return false;
        }
    }

    /**
    *
    * Simple function to sanitize the input
    *
    * @access private
    *
    * @param string
    *
    * @return string already sanitized
    *
    */
    private function clean_param($data)
    {
        $data = trim($data);
        $data = strip_tags($data);
        $data = addslashes($data);
        return $data;
    }

    /**
    *
    * Database error logging
    * Write the error to a file
    *
    * @param string
    *
    */
    private function log_err($err)
    {

        // Prepare the file for writing

        $fp = fopen('logs/error.txt', 'a') or die('Could not open file.');
        fwrite($fp, $err . PHP_EOL) or die('Could not write to file.');
        fclose($fp) or die('Failed to close the file.');
    }
}

Process Form:

<?php 
// processform.php

// Include the necessary files
require_once 'libraries/class-dbconnect.php';

if (isset($_POST['submit'])) {

    // Short variable names
    $fname = clean_data($_POST['fname']);
    $lname = clean_data($_POST['lname']);
    $email = clean_data($_POST['email']);
    $old_data = clean_data($_POST['olddata']);
    $new_data = clean_data($_POST['newdata']);
    $field = clean_data($_POST['field']);
    $id = clean_data($_POST['id']);
    $id = (int)$id;
    $sql = clean_data($_POST['query']);
    $operation = clean_data($_POST['operation']);

    // Instanciate the database class

    $pdo = new DbConnect('localhost', 'dbname', 'username', 'password');

    // Select an option

    switch ($operation) {
        case 'insert':

            if (empty($fname) || empty($lname) || empty($email)) {
                $error = 'Required fields are missing. <a href="index.php">Go back to the previous         page.</a>';
                echo $error, '<br>';
                exit;
            }

            $stmt = $pdo->insert('INSERT INTO tablename (first_name, last_name, email) VALUES         (:fname, :lname, :email)', array('fname' => $fname, 'lname' => $lname,         'email' => $email));

            $message = 'Successfully inserted ' . $pdo->get_row_count() . ' new data.' . '<br>';
            echo $message, '<br>';
            echo 'Last ID inserted: ', $pdo->get_last_inserted_id(), '<br>';
            echo '<a href="index.php">Go back to the form.</a>';
            break;
        case 'update':
            switch ($field) {
                case 'first_name':
                    $pdo->update('UPDATE tablename SET first_name = ? WHERE first_name = ?', array($new_data, $old_data));

                    if ($pdo->check_row_count()) {
                        echo 'Successfully changed your first name from ', $old_data, ' to ', $new_data, '. ', '<br>';
                        echo '<a href="index.php">Go back to the form.</a>';
                    } else {
                        echo 'Error updating the table. Check your input.', '<br>';
                        echo '<a href="index.php">Go back to the form.</a>';
                    }
                    break;
                case 'last_name':
                    $pdo->update('UPDATE tablename SET last_name = ? WHERE last_name = ?', array($new_data, $old_data));

                    if ($pdo->check_row_count()) {
                        echo 'Successfully changed your last name from ', $old_data, ' to ', $new_data, '. ', '<br>';
                        echo '<a href="index.php">Go back to the form.</a>';
                    } else {
                        echo 'Error updating the table. Check your input.', '<br>';
                        echo '<a href="index.php">Go back to the form.</a>';
                    }
                    break;
                case 'email':
                    $pdo->update('UPDATE tablename SET email = ? WHERE email = ?', array($new_data, $old_data));

                    if ($pdo->check_row_count()) {
                        echo 'Successfully changed your email from ', $old_data, ' to ', $new_data, '. ', '<br>';
                        echo '<a href="index.php">Go back to the form.</a>';
                    } else {
                        echo 'Error updating the table. Please check your input.', '<br>';
                        echo '<a href="index.php">Go back to the form.</a>';
                    }
                    break;
                default:
                    echo 'Unable to update.', '<br>';
                    exit;
                    break;
            }
            break;
        case 'select':
            echo '<h3>Current data</h3>', '<br>';
            get_data();
            echo '<a href="index.php">Go back to the form.</a>';
            break;
        case 'rowcount':
            $pdo->get_rows('SELECT * FROM tablename');
            $row = $pdo->get_row_count();
            echo 'There ', $row == 1 ? 'is ' : 'are ', $row, ' ', $row == 1 ? 'row ' : 'rows ', 'in the table.', '<br>';
            echo '<a href="index.php">Go back to the form.</a>';
            break;
        case 'delete':
            $pdo->delete('DELETE FROM tablename WHERE ID = ?', array($id));

            if ($pdo->check_row_count()) {
                echo 'Successfully deleted ID number ', $id, '.', '<br>';
                echo 'You deleted ', $pdo->get_row_count(), ' row in the table.', '<br>';
                $pdo->get_rows('SELECT * FROM tablename');
                echo 'Only ', $pdo->get_row_count(), ' rows left.', '<br>';
                echo '<a href="index.php">Go back to the form.</a>';
            } else {
                echo 'There\'s an error.', '<br>';
                echo '<a href="index.php">Go back to the form.</a>';
            }
            break;
        case 'query':
            $query = $pdo->query($sql);
            var_dump($query);
            break;
        case 'truncate':
            $pdo->get_rows('SELECT * FROM tablename');

            if (!$pdo->check_row_count()) {
                $error = 'Error truncating the table.' . '<br>';
                echo $error, '<br>';
                echo '<a href="index.php">Go back to the form.</a>';
                exit;
            } else {
                $pdo->fresh_table('tablename');
                echo 'Successfully truncated the table!', '<br>';
                echo 'Table now has ', $pdo->get_row_count(), ' rows.', '<br>';
            }
            echo '<a href="index.php">Go back to the form.</a>';
            break;
        default:
            echo 'Nothing selected.', '<br>';
            break;
    }

    $pdo->close_connection();
} else {
    echo 'You did not access the form.';
        exit;
}

function clean_data($data)
{

    // Sanitize the $data variable

    $data = trim($data);
    $data = strip_tags($data);
    $data = addslashes($data);
    return $data;
}

function get_data()
{
    global $pdo;

    $rows = $pdo->get_rows('SELECT * FROM tablename');

    if ($pdo->check_row_count()) {
        echo '<table id="tabledisplay">';
        echo '<tr><th>ID</th><th>First name</th><th>Last name</th><th>Email</th></tr>';

        foreach ($rows as $row) {
            echo '<tr><th>', $row['ID'], '</th>';
            echo '<td>', $row['first_name'], '</td>';
            echo '<td>', $row['last_name'], '</td>';
            echo '<td>', $row['email'], '</td></tr>';
        }
        echo '</table>';
    } else {
        echo '0 records found.', '<br>';
    }
}

index.php:

<!DOCTYPE html>
<html>
    <head>
        <meta charset="UTF-8">
        <title>Database Form</title>
    </head>
    <body>

        <form id="form" class="form" action="processform.php" method="post" role="form">
            <label for="fname"><span class="fieldlabel">Enter your first name:</span></label>
            <input type="text" id="fname" name="fname" maxlength="30">
            <label for="lname"><span class="fieldlabel">Enter your last name:</span></label>
            <input type="text" id="lname" name="lname" maxlength="30">
            <label for="email"><span class="fieldlabel">Enter your email:</span></label>
            <input type="email" id="email" name="email" maxlength="30">
            <label for="olddata"><span class="fieldlabel">Enter old data</span></label>
            <input type="text" id="olddata" name="olddata" maxlength="30">
            <label for="newdata"><span class="fieldlabel">Enter your new data here</span>        </label>
            <input type="text" id="newdata" name="newdata" maxlength="30">
            <label for="id"><span class="fieldlabel">Enter ID you want to delete</span></label>
            <input type="number" id="id" name="id" max-length="3">
            <label for="query"><span class="fieldlabel">Enter your query</span></label>
            <input type="text" id="query" name="query" max-length="50">
            <select name="field">
                <option value="first_name">First name</option>
                <option value="last_name">Last name</option>
                <option value="email">Email address</option>
            </select>
            <label for="operation"><span class="fieldlabel">What do you want to do?</span>        </label>
            <select id="operation" name="operation">
                <option value="insert">Insert</option>
                <option value="update">Update the table</option>
                <option value="delete">Delete from table</option>
                <option value="query">Query</option>
                <option value="select">Select</option>
                <option value="rowcount">Get the Number of rows</option>
                <option value="truncate">Truncate table</option>
            </select>
            <input type="submit" name="submit" value="Send">
        </form>
    </body>
</html>
\$\endgroup\$
  • 1
    \$\begingroup\$ is your indentation like this in the original, or does it result from pasting it here on CodeReview? \$\endgroup\$ – tim Oct 8 '14 at 12:24
  • \$\begingroup\$ What exactly is the problem? \$\endgroup\$ – Joey Oct 8 '14 at 12:27
  • \$\begingroup\$ There isn't any indentation (if this isn't a formating issue with your post, don't worry, leave it as it is). But bad indentation makes code very hard to read. If you'r using an IDE (which you should, it makes things a lot easier), it can automatically format your code for you. \$\endgroup\$ – tim Oct 8 '14 at 12:34
  • 1
    \$\begingroup\$ Thanks for the edits. Regarding indentation issues, I am actually using a screen reader. So formatting hasn't been given that much attention. But I try my best to follow the coding standards especially when I make my code public because I know very well how hard it is to read an unformatted code. \$\endgroup\$ – Joey Oct 8 '14 at 22:56
6
\$\begingroup\$

There's quite a book of code to go through here, so I'll probably update my answer every now and than, but for now, I'll leave you with this:

At first glance:

From the off, there are a couple of issues:

  • SRP: The Single Responsibility Principle is important. Your code violates this. The class you've posted is responsable for: connecting to the DB, preparing statements, fetching results and logging errors. That's too much for one class. This is also why PDO returns PDOStatement instances for the actual querying, and why there are logger classes all over the web for you to use
  • Be consistent, and be wise when sanitizing: clean_param is only being used on the constructor parameters. Why? Aren't I allowed to use slashes in my password? Aren't I allowed to connect to a socket (host=/path/to/db.sock)? You're making it impossible for me to use your class in those cases. At the same time, there is nothing in your class to sanitize user input to protect agains XSS attacks. A simple strip_tags call on user input can do wonders...
  • Coding standards: Please stick to the coding standards. They are important. You should fix your Indentation, and your method names are off
  • Doc-blocks: your doc-blocks specify the wrong types here and there, or don't specify an @return (get_row), or simply omit the @param array $data bit
  • A single class that, because of the SRP, should do just one thing should not control the flow of the application! A method that contains an exit statement controls the entire application in which it runs. It should be the other way around. If something goes awry, throw an exception, don't exit.
  • Again, because of the SRP, a DB class should not be allowed to generate output. Get rid of the echo statements ASAP. Methods return, the main scope (or the designated component - like the view) should take care of the output. It's not a task to be spread out throughout the entire code.
  • Don't catch fatal exceptions: At various points, you have a try-catch block that basically logs the exception that occurs. If I use your class, create a new instance, use the (rather dangerous) fresh_table method and it fails, I won't know about it until I actually check the logs. Something very bad has happened, something Exceptionally bad, that falls beyond the scope of your class. The user (who calls your methods) should be notified ASAP.
    Think of throwing an exception as breaching protocol: rather than to finish your job, and then return a response, you shout out to the user "Help, problem, I don't know what to do next, you fix it".
  • Like I've explained many times before here Your code is, essentially, pointless. PDO is a clear, easy to use and comprehensive API. Your not abstracting anything from the user, you're basically taking away functionality (like re-using prepared statements, setting connection attributes, the ability to close cursors and worst of all: the ability to use any adapter other than mysql, which you've hard-coded into the dsn string in your constructor).
  • As suggested in the comments: It would be best not to connect to the db in the constructor, but rather postpone connecting until you absolutely have to. (see example code in update below)

I'll go into more details on some of these issues shortly, but most of it is explained in a previous review I posted here. Read that review, and apply the critiques to your own code.

Update:
In the comments, xDaevax stated (correctly) that it's well worth wile not to connect to the DB in the constructor. A constructor's job is quite simple: it initializes a class' properties, nothing more. Connecting to the DB should be postponed as long as possible, an implement a form of lazy-loading. To avoid messing up this functionality in inheritance situations, you should make the connection property private and implement a lazy-loading final protected function getConnection method. final because it stops it from being overridden, protected because it shouldn't be visible from the outside and getConnection because that's what it does. An example:

class MyDB
{
    /**
     * @var PDO
     */
    private $db = null;

    /**
     * @var string
     */
    protected $dsn = null;

    /**
     * @var string
     */
    protected $user = null;

    /**
     * @var string
     */
    protected $pass = null;

    /**
     * @var array
     */
    protected $attributes = array(
        PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION
    );

    /**
     * constructor
     */
    public function __construct($dsn, $user, $pass, array $attr = null)
    {
        $this->dsn = $dsn;
        $this->user = $user;
        $this->pass = $pass;
        if ($attr)
            $this->attributes = $attr;
    }

    /**
     * Get prepared statement
     * @param string
     * @return PDOStatement
     */
    public function getStatement($query)
    {
        $db = $this->getConnection();
        return $db->prepare($query);
    }

    /**
     * @return bool
     */
    public function isConnected()
    {
        return ($this->db instanceof PDO);
    }

    /**
     * @ return PDO
     */
    final protected function getConnection()
    {
        if ($this->db === null)
        {//connect last-minute
            $this->db = new PDO(
                $this->dsn,
                $this->user,
                $this->pass,
                $this->attributes
            );
        }
        return $this->db;
    }
}
//usage
$inst = new MyDB('...', 'usr', 'pass');
var_dump($inst->isConnected());//false
$stmt = $inst->getStatement('SELECT foo, bar, FROM db.tbl WHERE id = :id');
var_dump($inst->isConnected());//true
//re-using a prepared statement:
$ids = [123, 4556];
$found = [];
foreach ($ids as $id)
{
    $stmt->execute([':id' => $id]);
    $found[] = $stmt->fetch(PDO::FETCH_OBJ);
    $stmt->closeCursor();
}
\$\endgroup\$
  • 1
    \$\begingroup\$ +1. All very good points. I would also add that the connection shouldn't be initialized in the constructor as it may not be needed yet and we don't want to have open connections just hanging out. As little work as possible should be done in the constructor and the connection should be encapsulated in some connect method. Also, connect and disconnect should not be publicly visible, but should be controlled as part of insert, select, update, or delete (so the lifetime of the connection can be managed more precisely). \$\endgroup\$ – xDaevax Oct 8 '14 at 15:55
  • \$\begingroup\$ @xDaevax Does that mean the constructor is ok to accept parameters but the connection method should be created outside the constructor? The original version of this code had a method called open_connection() and the constructor had no parameters. \$\endgroup\$ – Joey Oct 8 '14 at 19:40
  • \$\begingroup\$ @Joey Yes. You'll want the constructor to initialize everything (whether a specific value or a default) that is required for the class to operate. There are differing opinions about conditional logic in constructors (I avoid it), but invoking methods should be avoided generally. If someone extended your class and overrode the method called in the constructor, the side-effects would be unpredictable since the behavior would change. I would make the insert and other methods call connect and disconnect themselves so the user of your class doesn't have to remember (or forget) to do so. \$\endgroup\$ – xDaevax Oct 8 '14 at 19:49
  • 1
    \$\begingroup\$ I don't get this can you elaborate? "Also, connect and disconnect should not be publicly visible, but should be controlled as part of insert, select, update, or delete (so the lifetime of the connection can be managed more precisely)." What I understand from this suggestion is I have to open and close the connection continuously everytime I make a query. \$\endgroup\$ – Joey Oct 8 '14 at 21:20
  • \$\begingroup\$ @Joey: what xDaevax is hinting at is a form of lazy-loading. It's a technique that entails postponing initializing things like db connections as long as possible, so that you don't connect until you actually need to, I'll add a simple example to my answer \$\endgroup\$ – Elias Van Ootegem Oct 9 '14 at 6:49
3
\$\begingroup\$

Don't echo in a class

If you use echo in your database wrapper, the calling script has no influence on it. Maybe it doesn't want to echo, maybe it wants to echo something else, who knows.

The right way to manage these situations is to throw an exception.

echo unsanitized data

It looks like it's just a test script, but still: use htmlspecialchars when echoing user supplied data to prevent XSS attacks.

Unused Parameters

Methods shouldn't have unused parameters. For example $data of query is never used.

clean_param

Why are you cleaning the database credentials? This doesn't make very much sense, and it might break your code for certain passwords.

Emulate Prepared Statements

You should set

$this->con->setAttribute( PDO::ATTR_EMULATE_PREPARES, false );

for security purposes.

update and delete

update and delete just call insert. This doesn't seem quite right. In any case, I would remove these methods and use insert directly.

insert

insert probably should be called execute or parameterizedExecute, because that's what it does, and that's how it is used in the rest of the code.

query

I would remove this method completely. It's too easy to misuse it (not parameterizing user input, etc).

fresh_table

fresh_table should be called delete_table_data or similar (something that makes it really explicit that you are deleting the table data).

check_row_count

I would name this has_row or has_results. And you can rewrite it as return $this->get_row_count() > 0.

Style

As I said in the comments, you don't have any indentation, which makes your code very hard to read. Any IDE can fix this easily.

if (cond) { return true; } else { return false; }

if ($this->is_connected) {
return true;
} else {
return false;
}

can be written as

return $this->is_connected;
\$\endgroup\$
  • \$\begingroup\$ Turning off emulated prepares does mean you can't re-use named parameters, you'll also miss the query cache and there are syntax differences (emulated prepared statements are more forgiving) \$\endgroup\$ – Elias Van Ootegem Oct 8 '14 at 14:47

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.