PHP OOP: Duplicate Code in a Class

Question

I have a PHP class with two methods. One connects to a MySQL database for output, and the other connects to a MySQL database for input.

My question is, for both functions, I repeated the code for connecting to the database. What would be a better way to maybe have a third function in the class to connect to the DB and have the other two call the function to establish the connection, rather than repeat the code twice? I'm a PHP n00b trying to improve my OOP coding. Notice how I connected to the DB twice--using the exact same code:

class output_mysql {
var $db_name = 'database';
var $db_username = 'name';
var $db_password = 'mypassword';

function print_table_cell($tbl_name, $colm_name, $array_index_num) {
    try {
        $pdo = new PDO("mysql:host=localhost;dbname=$this->db_name", $this->db_username, $this->db_password);
        $pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
    }
    catch (PDOException $e) {
        $error = 'Unable to connect to the database server.';
        include 'output_mysql_error.php';
        exit();
    }
    try {
        $sql = "SELECT $colm_name FROM $tbl_name";
        $result = $pdo->query($sql);
    }
    catch (PDOException $e) {
        $error = 'Error fetching content: ' . $e->getMessage();
        include 'output_mysql_error.php';
        exit();
    }
    while ($row = $result->fetch()) {
        $all_content[] = $row["$colm_name"];
    }
    echo $all_content[$array_index_num];
}

function update_content($tbl_name, $colm_name, $error_message_text, $id_num) {
    try {
        $pdo = new PDO("mysql:host=localhost;dbname=$this->db_name", $this->db_username, $this->db_password);
        $pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
    }
    catch (PDOException $e) {
        $error = 'Unable to connect to the database server.';
        include 'output_mysql_error.php';
        exit();
    }
    try {
        $sql = 'UPDATE website_content SET
            content = :content,
            date_added = CURDATE()
            WHERE id = :id';
    $s = $pdo->prepare($sql);
    $s->bindValue(':content', $error_message_text);
    $s->bindValue(':id', $id_num);
    $s->execute();
    }
    catch (PDOException $e) {
        $error = 'Error: ' . $e->getMessage();
        include 'output_mysql_error.php';
        exit();
    }
}
}

The answer can be found in your question: have a third function in the class to connect to the DB and have the other two call the function to establish the connection — Vlad Preda
– Vlad Preda, Commented Jan 25, 2013 at 13:34
Note sure that this is suppose to do but please be aware you have the chance for SQL injection in your code as it is ... — TheKingDave
– TheKingDave, Commented Jan 25, 2013 at 13:42
KingDave, doesn't the prepared statement in the second method protect me against SQL injection? The first method is just doing output. — GTS Joe
– GTS Joe, Commented Jan 25, 2013 at 15:09

PeeHaa · Accepted Answer · 2013-01-25 13:45:47Z

This question is tagged [oop], but the code in it is far from OOP.

Your methods are doing waaaaaaaaaaaaay too much. What you should do is inject the database connection into the constructor of the output_mysql class (which is a terrible name btw).

namespace App\Page;

class Content
{
    private $dbConnection;

    public function __construct(\PDO $dbConnection)
    {
        $this->dbConnection = $dbConnection
    }

    public update($id, $content)
    {
        $stmt = $this->dbConnection->prepare('UPDATE website_content SET content = :content, date_added = CURDATE() WHERE id = :id');
        $stmt->execute([
            'id' => $id,
            'content' => $content,
        ]);
    }

}

$dbConnection = new \PDO("mysql:host=localhost;dbname=$this->db_name", $this->db_username, $this->db_password);
$dbConnection->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
$dbConnection->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);

$pageContent = new \App\Page\Content($dbConnection);
$pageContent->update(1, 'new content');

If you have a method called print_table_cell you are probably doing OOP wrong, because it probably means you code is doing too much and probably violates the Single Responsibility Principle. I mean a class in almost all circumstances would never need to be able to access any column of just any table.

epicdev · Accepted Answer · 2013-01-25 14:03:18Z

2

class Model
{
    protected $pdo;

    /**
     * Inject the pdo driver in the model.
     */
    public function __construct(PDO $pdo)
    {
        $this->pdo = $pdo;
    }

    public function print_table_cell($tbl_name, $colm_name, $array_index_num)
    {
        // Use the pdo object $this->pdo
    }
}

// Create the connection
$dbName = '';
$dbUsername = '';
$dbPassword = '';

$pdo = new PDO("mysql:host=localhost;dbname=$dbName", $dbUsername, $dbPassword);
$pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

// Create your model and inject the pdo object.
$model = new Model($pdo);
$model->print_table_cell() ...

As said above you need to use prepared statements, because PDO will escape the values which prevents SQL injections. But anyway all input data must be filtered : you have some basic filters http://php.net/manual/fr/function.filter-var.php.

The model class should only interact with the database, and doesn't print anything.

For priting output you can use a so called View class that get data from the model and displays it.

class View
{
    protected $model;

    public function __construct(Model $model)
    {
        $this->model = $model;
    }

    public function render()
    {
        echo $this->model->getData();
    }
}

class Model
{
    protected $pdo;

    public function __construct(PDO $pdo)
    {
        $this->pdo = $pdo;
    }

    public function getData()
    {
        // Do your query here with $this->pdo and prepared statement.
        // and return the data
    }
}

$pdo = new PDO(...);
$model = new Model($pdo);
$view = new View($model);
$view->render();

edited Jan 25, 2013 at 14:03

answered Jan 25, 2013 at 13:42

epicdev

9224 silver badges10 bronze badges

1 Comment

Gordon Over a year ago

+1 for advising on Prepared Statements. However, Model is a Layer and not some class and Model does not equal database either. Database is Persistence Layer. That's just one layer inside the Model Layer. The Model is all the classes not belonging to the Presentation Layer.

Collectives™ on Stack Overflow

PHP OOP: Duplicate Code in a Class

2 Answers 2

Comments

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related