Code Review DB Connection

PHP
1

How can this code be improved? I imagine this class will need to connect to a variety of databases so I’m trying to make it as DRY as possible.

login page includes the db-config.php page and then calls this

$login=new Login();
$login->reference("user_login");

db-config page is here.

<?php
class Login
{
  public function reference($event)
  {
      $this->connectDB($event);
  }
  private function connectDB($login)
  {
    $host="localhost";
    $username="asdf";
    $password="asdf";
    $db="codefund_".$login;
    
    try
    {
      $conn = new PDO('mysql:host='.$host.';dbname='.$db, $username, $password);
      if($conn)
        echo "conn good";
      //close connection
      $conn=null;
    }
    catch (PDOException $e)
    {
      print "Error!: " . $e->getMessage() . "<br/>";
      die();
    }
  }
}
?>
2

This is the connection class that I generally use:

class Database {

  private $_connection;
  // Store the single instance.
  private static $_instance;

  // Get an instance of the Database.
  // @return Database: 
  public static function getInstance() {
    if (!self::$_instance) {
      self::$_instance = new self();
    }
    return self::$_instance;
  }

  // Constructor - Build the PDO Connection:
  public function __construct() {
    $db_options = array(
        /* important! use actual prepared statements (default: emulate prepared statements) */
        PDO::ATTR_EMULATE_PREPARES => false
        /* throw exceptions on errors (default: stay silent) */
        , PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION
        /* fetch associative arrays (default: mixed arrays)    */
        , PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC
    );
    $this->_connection = new PDO('mysql:host=' . DATABASE_HOST . ';dbname=' . DATABASE_NAME . ';charset=utf8', DATABASE_USERNAME, DATABASE_PASSWORD, $db_options);
  }

  // Empty clone magic method to prevent duplication:
  private function __clone() {
    
  }

  // Get the PDO connection:    
  public function getConnection() {
    return $this->_connection;
  }

}

/* General PDO connection string */
$db = Database::getInstance();
$pdo = $db->getConnection();

Maybe this will give you help:

3

Thank you but that doesn’t really tell me what’s wrong with mine or what “best practices” aren’t being followed, etc. I appreciate the code sample to review though.

4

Well, for best practice this kind of skirts that for some people say it uses singleton, yes it does but it does it in a secure way.

Take a look a the getConnection method and static funtion…if you use method don’t forget the __clone()

5

Thank you - what is “singleton”? I’ve seen it f loated around here but I’m not sure what it is.

Also I’m going over your code but why do you first return an instance of the database?

Could you, in plain terms, go over that code for me? Your code @Pepster

6

Really don’t know 100 percent, but I here the true eggheads really talk it down for it’s supposedly a “bad” way to do it. Though I had one person say that it was OK to do for simple tasks like connection to a database table for example. Someone here probably could explain it a little bit better.

7

Thank you for that. I just googled and got some explanation. I did edit my above post if you could look at that.

8

I’ve updated my code

<?php
class Login
{
  protected $host="localhost";
  protected $username="sdfa";
  protected $password="sfdas";
  protected $db="codefund_";

  public function connectDB($login)
  {
    try
    {
      $conn = new PDO('mysql:host='.$this->host.';dbname='.$this->db.$login, $this->username, $this->password);
    }
    catch (PDOException $e)
    {
      print "Error!: " . $e->getMessage() . "<br/>";
      die();
    }
  }
  public function closeConnection()
  {
    $this->conn=null;
  }
}
?>

Calling it…

$login=new Login();
$login->connectDB("user_login");
$login->closeConnection();
9

Hi Ryan,

A couple of thoughts about your class:

Don’t instantiate the DB connection within your class. This has several drawbacks:

  • A new DB connection will be created every time you create an instance of Login.
  • Other objects can’t reuse the DB connection.
  • The Login class is can’t be reused in another project without modifying the code to change the DB credentials (plus, if you have other classes that create their connection like this, you’ve got multiple places in the code that you need to change if the credentials change).

It would be better to pass a pre-configured PDO object into your class via the constructor.

Don’t use die(). The whole point of using try/catch blocks is to enable your application to recover from exceptions, and using die just kills your app entirely. If you’re not going to handle an exception in some way or replace the PDO exception with a custom one then you’re better off letting the exception bubble up to be handled at a higher level (where you could at least display a user-friendly error message).

1 Like
10

Hello,

I’m pretty new to PDO and using OOP in general. Would you mind giving me code samples that illustrate how I could restructure my code? I’ll attempt to rewrite from there.

11

Ok I think I reworked this a good bit. THoughts? I don’t see (from browsing the Internet) how I would go about selecting my db and stuff

<?php
class Login
{
  protected $db_handler;
  protected $username;
  protected $password;
  public $validCredentials;

  public function __construct(PDO $db, $email, $pass) 
  {
    $this->db_handler = $db;
    $this->username = $email;
    $this->password = $pass;
    $this->connect();
  }
  public function connect()
  {
    $validCredentials=$this->checkCredentials();
    if($validCredentials)
    {
      echo "adsf";
    }
  }
  protected function checkCredentials()
  {
    return true;
  }
  public function closeConnection()
  {
    $this->conn=null;
  }
}
?>

Calling

$pdo=new PDO('mysql:dbname=dbnamehereadsfsdf', 'codefund', 'pwhere!');
$login = new Login($pdo, "admin@codefundamentals.com", "password");
12

I like fretburner would also question the usefulness of such a class instead of passing the PDO instance via composition to classes which require it such as; models. The other issue I have is hard coding credentials lacks portability between multiple environments. In *most cases differing environments will have contrasting authentication credentials. Therefore, it would be better to perhaps move the credentials out of the class and into files perhaps yaml. The application code could than use a form of discovery to find the correct credentials based on the current, active environment. This is how all the modern frameworks work.

13

@oddz

Pretty much 90% of what you just said went completely over my head.

14

Any class that requires the PDO would have a constructor parameter for it.

ie.

class Whatever {
  protected $db;
  public function __construct(PDO $db) {
    $this->db = $db;
  }
}
15

Any class that requires the PDO would have a constructor parameter for it.

ie.

class Whatever {
  protected $db;
  public function __construct(PDO $db) {
    $this->db = $db;
  }
}

Wrapping the PDO instance in another class has very little usefulness unless your creating an Abstracted API. One which the persistent storage mechanism (think MySQL, Mongo, etc) can be changed. However, if you will only ever be using PDO than just passing PDO as an argument to classes that requires it through composition would be fine.

I don’t know what your end game is here but both Symfony and Laravel will teach you how to build a modern, well architected web application faster than stumbling through building your own will fyi.

1 Like
16

Thank you oddz. I find I do actually learn faster by coding in the raw language. It’s easier to look stuff up and adapt code and ideas. No offense but I’ll shy away from frameworks for now.

I apologize for constantly changing my code but here is the “final” version that should just be missing actual comparing values in the database / removing echos, etc. Thoughts? Initial problems?

Calling

$pdo=new PDO('mysql:dbname=dbnamethingy', 'user', 'pw!');
$login = new Login($pdo, "email@email.com", "pw");
$login->closeConnection;

db-config

<?php
class Login
{
  protected $db_handler;
  protected $username;
  protected $password;
  public $validCredentials;

  public function __construct(PDO $db, $email, $pass) 
  {
    $this->db_handler = $db;
    $this->username = $email;
    $this->password = $pass;
    $this->connect();
  }
  public function connect()
  {
    $validCredentials=$this->checkCredentials();
    if($validCredentials)
    {
      echo $validCredentials['username'];//temporary
    }
  }
  protected function checkCredentials()
  {
    $userQuery = $this->db_handler->prepare('SELECT * FROM Users WHERE username=?');
    $userQuery->execute(array($this->username));
    if ($userQuery->rowCount() > 0)
    {
      $getUserDetails = $userQuery->fetch(PDO::FETCH_ASSOC);
      return $getUserDetails;//will validate before return later
    }
    return false;
  }
  public function closeConnection()
  {
    $this->conn=null;
  }
}
?>
17

Are you thinking of perhaps extending PDO to do things like count the number of queries run or to simplify places where you’re doing say bulk inserts with prepared statements (so that where you’re trying to do the bulk insert it just passes the query and the array of data to be inserted to the extended class)?

18

It’s certainly possible that that scenario will happen. I’m trying to make this so if I decide to do something with this, this code will work appropriately. Does that make sense? This should be able to adapt to what I want to throw at it.

19

Eh actually with this class (thinking it over) this really is just a login system that I want adaptable for future use and if this does go open source, it’s easy for others to modify with their information perhaps.

Other classes might extend PDO but this one does what I want and seems to be pretty clean. However I’m no expert so I do wish to have it reviewed. Perhaps I’m breaking some OOP rule.

20

Do you think you will always be working with MySQL?
eg. if you decided to work with SQLite you would need to hard code an almost identical script but with
$pdo = new PDO('sqlite:' . $this_db);
So you may want to pass an “engine” to __construct too