-1
\$\begingroup\$

This is my wrapper, how can i improve this? (With a Singleton too)

<?php 

define("DB_HOST", "");
define("DB_USER", ""); 
define("DB_PASS", ""); 
define("DB_DATA", "");

class Connection extends PDO{
        public function __construct($host = DB_HOST , $data = DB_DATA , $user = DB_USER , $pass = DB_PASS){
            try {
                parent::__construct("mysql:host=$host;dbname=$data;" , $user , $pass);
            } catch(PDOException $e){
                print "Error!: " . $e->getMessage() . "<br/>";
                die();
            }
        }
        public function query($query , array $valori = [] ){
            try {
                $res = parent::prepare($query);
                if($res === false ){
                    throw new \PDOException("#01");
                }
                if($res->execute($valori) === false ){
                    throw new \PDOException("#02");
                }
                return $res ; 
            } catch(PDOException $e) {
                print "Error!: " . $e->getMessage() . "<br/>";
                die();
            }   
        }
    }
?>
\$\endgroup\$
3
\$\begingroup\$

Honestly, you can improve your application by discarding this wrapper. You are not going to derive any value from it.

You can find similar discussion around this very common Codereview topic in questions such as those I have listed below.

Class for reducing development time

Simple PDO database class in PHP

Phishing Project Error Logging

What you should hopefully take away from this is that you are adding an unnecessary level of abstraction around an existing database abstraction (PDO) and adding no additional value to it. Typically one might write database-related classes to do things like:

  • connection management (like a Singleton or connection pool implementation)
  • adding "model" capabilities (i.e. ORM, active record pattern, etc.)
  • adding fluent, natural language query capabilities ($db->select('field')->from('table')->where('id = ?') or similar).

You are doing none of that. You are just obfuscating the underlying PDO implementation from the caller, while simultaneously leaking implementation details outside the class (you are throwing PDOException for example).

Some specific comments on code

  • Consider moving your database configuration to outside of your code. There is no reason login credentials should be present in any of your code. Ideally you derive these from environmental configuration.
  • Why are you defining variables in your parameters here => public function __construct($host = DB_HOST , $data = DB_DATA , $user = DB_USER , $pass = DB_PASS)? Why do you even need parameters if these values are defined as constants? You could build your DB connection string in your constructor directly from the constants. Are you really expecting to have to make dynamic/runtime assignment for these host and login credentials as you are trying to allow for?
  • Don't echo out error messages to standard out (i.e. die('message')). These errors should be logged and an appropriate exception raised to inform the caller of the error. It should not be the place of this class to determine end-user messaging. The calling code is better positioned to do this.
  • You should validate parametric data passed into public methods. Here on your query() method you do nothing to valdiate you have a reasonable query string to work with. You should validate and throw appropriate InvalidArgumentException or similar if validation fails. You don't to try to make queries against the database when an empty string is passed, for example.
\$\endgroup\$
1
\$\begingroup\$

For what purposes do you need PDO wrapper?

On my point of view:

  1. Store config in special config object/array or invironments, like dotenv, so you can get settings with getenv function
  2. You also may separate db type (mysql) from config string
  3. Try to name variables obviously:
    • $user => $username
    • $valori => $queryData
    • $res => $preparedStatement
  4. You may particularly specify error messages instead of "#01", "#02"
  5. If it is custom library, throw exceptions instead of die
\$\endgroup\$

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