Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

What would be the preferred method for handling errors when using the PDO to connect to a MySQL database? I was using mysqli_error($this->pdo), but I heard this was bad practice. Any feedback would be much appreciated.

class Database
{
    /*
     * Database properties
     */
    private $server   = 'localhost';
    private $username = 'root';
    private $password = 'root';
    private $database = 'posts';
    public $pdo;
    public static $instance;

    /**
     * Static method used to instantiate singleton
     * @return [object] Database object
     */
    public static function getInstance()
    {
        if (!isset($instance)) {
            Database::$instance = new Database();
        }
        return Database::$instance;
    }

    /**
     * Connect to DB when class is instantiated
     */
    private function __construct() 
    {
        $this->connect();
    }

    /**
     * Connects to the database
     */
    private function connect()
    {   
        // Assign dsn string
        $dsn = 'mysql:host=' . $this->server . ';dbname=' . $this->database;

        // Assign connection to $pdo
        $pdo = new PDO($dsn, $this->username, $this->password);

        // $pdo inside the function is added to the objects parameter pdo
        else {
            $this->pdo = $pdo;
        }
    }

    /**
     * Queries database for the argument $sql
     * @param  string $sql
     * @return Sql Query $statement
     */
    public function query($sql)
    {   
        // Prepare SQL query
        $statement = $this->pdo->prepare($sql);

        // return mysql query array
        else {
            return $statement;
        }
    }

    /**
     * Queries for post of id $id
     * @param  [$id] Post ID
     * @return [array]
     */
    public function getPost($id)
    {
        // Query
        $stmt = $this->query("SELECT * FROM posts WHERE id=(:id)");

        // Binds $id parameter to the PDO
        $stmt->bindParam(':id', $id);

        // Execute query
        $stmt->execute();

        // Create an empty array for the posts
        $posts = array();

        while($row = $stmt->fetch()) {
            // Data is fetched from the database as an array
            $posts[] = $row;
        }   

        // Return post in an array
        return $posts;
    }   

}
share|improve this question
    
Have you fully tested this code? I'm noticing several instances of elses without the initial if ()s. Technically, shouldn't that produce an error? –  Alex L Jan 1 at 20:26
    
Oops thats will because I just took out the mysqli_error() handler. –  zia grosvenor Jan 1 at 21:02
    
    
Again, please do not modify the original code after receiving answers. –  Jamal Jan 1 at 21:16
    
Ah okay apologies I only just started using code review. –  zia grosvenor Jan 1 at 21:18

1 Answer 1

up vote 1 down vote accepted

From a first pass over - all you are doing are if else without any throw or inspection as to what possible errors that the PDO might be tossing out.

Thats bad because on production servers - I would assume that you won't be turning on error/warning reporting but will be logging them.

Also as @Alex L mentioned - you are missing a IF statement that checks if $pdo == null then throw exception. However its better with a try/catch

try { 
    $pdo = new PDO($dns, $username, $password, $options); 
} catch (PDOException $e) { 
    throw new pdoDbException($e); 
} 
share|improve this answer
    
Thanks, I will be using try/catch from now on. –  zia grosvenor Jan 1 at 21:14

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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