Looking for coding advice

[Azarian]

Looking for opinions if I am on the right track with my coding and how i can improve what I have so far.  A little back ground first.  I have a couple of applications I made back in the day in php, before php really followed OOP.  A about 2 years ago i tried learning OOP practices to update a couple of my projects because a buddy of mine wanted to use them and for what ever reason my brain wasn't gettin it so I kinda pushed all my coding projects aside and haven't looked at them since.  I got the coding bug again and I want to get some of these old projects redone so i can move on the bigger and better ideas I have in mind.  I think what made my brain fry last time I tried to grasp this concept was i spent to much time reading different coders ideas on how to make perfect code than reading the next guy's and what he had to say was completely different than the next if you know what I mean.  After looking at many examples of database classes this is what i came up with so far.  There was a lot of code that was more elegant and more verastile but I opted for a more simplistic approach.

class DB {

	private static $_instance = null;
	
	private $_pdo;
	private $_query;
	private $_error = false;
	private $_results;
	private $_count = 0;
	private $_db_options = array(
            PDO::ATTR_PERSISTENT    => true,
            PDO::ATTR_ERRMODE       => PDO::ERRMODE_EXCEPTION
			);	
	
	private function __construct() {
		try {
			$this->_pdo = new PDO('mysql:host=' . Config::get('database/hostname') . ';dbname=' . Config::get('database/db_name'), Config::get('database/username'), Config::get('database/password'), $this->_db_options);
		} catch(PDOException $e) {
			die($e->getMessage());
		}
	}
	
	public static function getInstance() {
		if(!isset(self::$_instance)) {
			self::$_instance = new DB();
		}
		return self::$_instance;
	}
	
	private function bind($bind) {		
		foreach($bind as $param => $value){
			$this->_query->bindValue($param, $value);
		}
	}	
	
	public function select($column, $table, $where, $bind) {
		$sql="SELECT {$column} FROM {$table} WHERE {$where}";
		$this->_query = $this->_pdo->prepare($sql);
		$this->bind($bind);		
		return $this->_query;
	}
	
	public function selectall($table, $where, $bind) {
		$sql="SELECT * FROM {$table} WHERE {$where}";
		$this->_query = $this->_pdo->prepare($sql);
		$this->bind($bind);
		return $this->_query;
	}
	
	public function delete($table, $where, $bind) {
		$sql="DELETE FROM {$table} WHERE {$where}";
		$this->_query = $this->_pdo->prepare($sql);
		$this->bind($bind);
		return $this->_query;
	}
	
	public function insert($table, $fields, $values) {
		$sql="INSERT INTO {$table} ({$fields}) VALUES ({$values})";
		$this->_query = $this->_pdo->prepare($sql);
		$this->bind($bind);
		return $this->_query;
	}
	
	public function update($table, $fields, $values) {
		$sql="UPDATE {$table} SET({$fields}) WHERE ({$values})";
		$this->_query = $this->_pdo->prepare($sql);
		$this->bind($bind);
		return $this->_query;
	}
	
	public function resultset() {
		$this->_query->execute();
		return $this->_query->fetchAll(PDO::FETCH_ASSOC);
	}
	
	public function result() {		
		$this->_query->execute();
		return $this->_query->fetch(PDO::FETCH_ASSOC);
	}

	public function rowCount() {
		return $this->_query->rowCount();
	}

	public function lastInsertId() {
		return $this->_pdo->lastInsertId();
	}
	
}

Now its how I am using it is where i am unsure if the approach i choose is the right way of going about it or not.  This next class i am calling User_DataManager class its function is provide the User class Data from the Database.  I don't want my User class to have any knownledge of the database's goings on.  That way the only place I need to change that info will be in this class only.

class User_DataManager {

	private $_db = null;	
	
	public function __construct() {
		$this->_db = DB::getInstance();
	}
	
	public function existing_emailaddress($email){	
		
		$table = 'tbl_users';
		$where =  'fld_emailaddress = :email';
		$bind = array(':email' => $email);
		
		$this->_db->selectall($table, $where, $bind);
		
		return $this->_db->result();
	
	}
	
	public function banned_emailaddress($email){	
		
		$table = 'tbl_banned_emailaddresses';
		$where =  'fld_emailaddress = :email';
		$bind = array(':email' => $email);
		
		$this->_db->selectall($table, $where, $bind);		
		
		return $this->_db->result();
	
	}
	
	public function existing_username($username){	
		
		$table = 'tbl_users';
		$where =  'fld_username = :username';
		$bind = array(':username' => $username);
		
		$this->_db->selectall($table, $where, $bind);
		
		return $this->_db->result();
	
	}
	
	public function banned_username($username){	
		
		$table = 'tbl_banned_usernames';
		$where =  'fld_banned_username = :username';
		$bind = array(':username' => $username);
		
		$this->_db->selectall($table, $where, $bind);		
		
		return $this->_db->result();
	
	}

}

To me this seeems like a lot of redudant code and this is just a small part of what is going on.  Am I on the right track?  Is there a better way I can or should be doing this?

[ignace]

Singleton is an anti-pattern instead pass the DB object to your User_DataManager through the constructor. Use a dependency injection container to resolve your dependencies.

 

You can use Pimple for example.

 

This allows you to decouple your query builder DB from the actual database connection PDO.

 

$pimple['config_file'] = '/path/to/config.ini';

$pimple['config'] = function($c) {
  return Config::fromIni($c['config_file']);
};

$pimple['db'] = function($c) {
  $cfg = $c['config'];
  $db = $cfg['db'];
  return new PDO($db['dsn'], $db['user'], $db['pass'], $db['options']);
};

$pimple['query_builder'] = function($c) {
  return new DB($c['db']);
};

Also avoid using die() inside classes and instead use exceptions which can be handled and formatted in a nice 404 or 503 message to the user. Edited February 12, 2015 by ignace

[Azarian]

Singleton is an anti-pattern instead pass the DB object to your User_DataManager through the constructor. Use a dependency injection container to resolve your dependencies.

 

You can use Pimple for example.

 

This allows you to decouple your query builder DB from the actual database connection PDO.

 

Also avoid using die() inside classes and instead use exceptions which can be handled and formatted in a nice 404 or 503 message to the user.

 

 

I have never heard about this dependency injection or singleton being the anti pattern.  I been googleing this for the last hour or two, to figure out what you meant and found it intriguing.  Why is this singleton pattern for your DB class pretty much the standard that is plastered all over the internet?  This dependency injection seems closer to the concept that I was using before I tried to go OOP with my code.  With all these singleton examples you see everyone putting there CRUD or query building in the same class as the db connection.  With dependency injection it looks like the DB class is for connection only and nobody is really using any kind of query building there just using the full blown code where ever they need it ?

[Strider64]

Dependency interjection is pretty neat for the class(es) don't have to rely on another class(es). 

 

Here's kind of a silly example:

<?php
class Customer {
  private $firstName;
  private $lastName;
  
  public function __construct($firstName, $lastName) {
    $this->firstName = $firstName;
    $this->lastName = $lastName;
  }
  
  public function getFirstName() {
    return $this->firstName;
  }
  
  public function getLastName() {
    return $this->lastName;
  }
}

class Store {
  private $customer;
  private $product;
  
  public function __construct($product, Customer $customer) {
    $this->customer = $customer;
    $this->product = $product;
  }
  
  public function getCustomer() {
    return $this->customer;
  }
  
  public function getProduct() {
    return $this->product;
  }
}

$sales = new Store("iPhone 6", new Customer("Bill", "Gates"));

$customer = $sales->getCustomer();

echo $customer->getFirstName() . ' ' . $customer->getLastName() . ' snuck into an Apple Store an bought an ' . $sales->getProduct() . "!<br>\n";
echo '<br>';
echo $sales->getCustomer()->getFirstName() . ' ' . $sales->getCustomer()->getLastName();

$person = new Customer("Steve", "Jobs");

$store = new Store("MicroSoft Windows", $person);

//var_dump($store);
echo '<br>';
echo $store->getCustomer()->getFirstName() . ' ' . $store->getCustomer()->getLastName() . ' Product : ' . $store->getProduct() . "<br>\n"; 

It's great for testing modules, I would guess it would also be great for a large project where you can have multiple people working on it. 

Edited February 13, 2015 by Strider64

[Azarian]

So if I am understanding this, on my DB class if i delete the singleton setup and change my construct to something more like this?  This is more like what I should be doing?

public function __construct($db_settings = array()) {
		$db_settings['db_type']=$db_type;
		$db_settings['hostname'] =$hostname;
		$db_settings['username'] =$username;
		$db_settings['password'] =$password;
		$db_settings['db_name'] =$db_name;
		$db_settings['db_options'] =$db_options;
		
		try {
			$this->_pdo = new PDO( . $db_type . ':host=' . $hostname . ';dbname=' . $db_name, $username, $password, $db_options);
		} catch(PDOException $e) {
			$this->error = $e->getMessage();
		}
	}

Edited March 4, 2015 by Azarian