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();
    }
}
}

Answer 1

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();

Answer 2

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.