Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link
  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:

http://stackoverflow.com/questions/6366661/php-pdo-parameterised-query-for-mysql https://stackoverflow.com/questions/6366661/php-pdo-parameterised-query-for-mysql

  1. 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.

  2. 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');
  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:

http://stackoverflow.com/questions/6366661/php-pdo-parameterised-query-for-mysql

  1. 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.

  2. 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');
  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

  1. 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.

  2. 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');
Post Migrated Here from stackoverflow.com (revisions)
Source Link
Zak
Zak
  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:

http://stackoverflow.com/questions/6366661/php-pdo-parameterised-query-for-mysql

  1. 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.

  2. 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');
default

AltStyle によって変換されたページ (->オリジナル) /