patternphpMinor
PDO (MySQL) connection and query class, safety and logic
Viewed 0 times
querylogicmysqlsafetyandclassconnectionpdo
Problem
For the last few day I tried to figure out how to work with MySQL and PDO. Although I've tried to read a lot about the subject, there are still a lot of things I don't understand.
Because of this lack of knowledge, I can't really judge this code (or other example code on-line) on its safety and logic, and therefore was hoping I could get some feedback on it.
The class makes a connection and returns a query:
Using the class:
Because of this lack of knowledge, I can't really judge this code (or other example code on-line) on its safety and logic, and therefore was hoping I could get some feedback on it.
The class makes a connection and returns a query:
class Connection
{
private $username = "xxxx";
private $password = "xxxx";
private $dsn = "mysql:host=host;dbname=name_db";
private $sql;
private $DBH;
function setSQL($sql){
$this->sql = $sql; //if $sql is not set it troughs an error at PDOException
}
public function query()
{
//the connection will get established here if it hasn't been already
if (is_null($this->DBH))
try {
$this->DBH = new PDO( $this->dsn, $this->username, $this->password );
$this->DBH->setAttribute( PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION );
//query
$result = $this->DBH->prepare($this->sql);
$result->execute();
return $result->fetch(PDO::FETCH_ASSOC);
} catch(PDOException $e) {
echo "I'm afraid I can't do that1.";
//file_put_contents('PDOErrors.txt', $e->getMessage(), FILE_APPEND);
}
}
//clear connection and variables
function __destruct(){
$this->DBH = null;
}
}Using the class:
$sql = "SELECT * from stock WHERE id = 302";
$test = new Connection();
$test->setSQL($sql);
echo $test->query();Solution
1) When using PDO, you want to get the benefits of automatic parameter escaping.
By wrapping your PDO class you are limiting it's functionality. Check out this question for a better example:
https://stackoverflow.com/questions/6366661/php-pdo-parameterised-query-for-mysql
2) Every time you run a query you are making a new PDO instance. It might be better to have an application resource pool that you can call to get a preconfigured db handle.
3) You wrote some sql to be able to fetch a stock by ID, that should probably be functionality that is reusable.
combining 1 & 2 & 3 .. code would probably look better as something like this:
your codeBlock then becomes:
By wrapping your PDO class you are limiting it's functionality. Check out this question for a better example:
https://stackoverflow.com/questions/6366661/php-pdo-parameterised-query-for-mysql
2) Every time you run a query you are making a new PDO instance. It might be better to have an application resource pool that you can call to get a preconfigured db handle.
3) You wrote some sql to be able to fetch a stock by ID, that should probably be functionality that is reusable.
combining 1 & 2 & 3 .. code would probably look better as something like this:
class ApplicationResourcePool
{
static var $_dbHandle;
private static $_dbConfig = array(
'dsn' => '',
'username' => '',
'password' => '',
);
public static getDbHandle(){
if(self::$_dbHandle == null){
self::$_dbHandle = new PDO(
self::$_dbConfig['dsn'] ,
self::$_dbConfig['username'],
self::$_dbConfig['password']
);
}
return self::$_dbHandle;
}
}
class StockMapper
{
protected $_dbh;
public __construct($dbh = null)
{
if($dbh == null){
$this->_dbh = ApplicationResourcePool::getDbHandle();
} else {
$this->_dbh = $dbh;
}
}
public getStockById($stockId){
$sth=$this->_dbh->prepare("SELECT * from stock WHERE id = :stockId");
$sth->bindParam(":stockId", $stockId);
$sth->execute();
$result = $sth->fetch(PDO::FETCH_ASSOC);
return $result[0];
}
}your codeBlock then becomes:
$stockMapper = new StockMapper();
$stockData = $stockMapper->getStockById('302');Code Snippets
class ApplicationResourcePool
{
static var $_dbHandle;
private static $_dbConfig = array(
'dsn' => '',
'username' => '',
'password' => '',
);
public static getDbHandle(){
if(self::$_dbHandle == null){
self::$_dbHandle = new PDO(
self::$_dbConfig['dsn'] ,
self::$_dbConfig['username'],
self::$_dbConfig['password']
);
}
return self::$_dbHandle;
}
}
class StockMapper
{
protected $_dbh;
public __construct($dbh = null)
{
if($dbh == null){
$this->_dbh = ApplicationResourcePool::getDbHandle();
} else {
$this->_dbh = $dbh;
}
}
public getStockById($stockId){
$sth=$this->_dbh->prepare("SELECT * from stock WHERE id = :stockId");
$sth->bindParam(":stockId", $stockId);
$sth->execute();
$result = $sth->fetch(PDO::FETCH_ASSOC);
return $result[0];
}
}$stockMapper = new StockMapper();
$stockData = $stockMapper->getStockById('302');Context
StackExchange Code Review Q#5735, answer score: 3
Revisions (0)
No revisions yet.