current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

For the last few day I tried to figure out how to work with mysql and PDO. All tough I've tried to read a lot about the subject, still there are a lot of things I don't understand.

Because of this lack of knowledge I cant 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;
}

}

use the class

$sql = "SELECT * from stock WHERE id = 302"; 
$test = new Connection();
$test->setSQL($sql);
echo $test->query();
share|improve this question
add comment

migrated from stackoverflow.com Nov 1 '11 at 15:03

This question came from our site for professional and enthusiast programmers.

2 Answers

up vote 1 down vote accepted

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:

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');
share|improve this answer
    
perfect!, that seems like a more logic and better way to handle this. Would you have any advice how to handle the PDOException in this example? Would you add it as a function('s) to getStockById? –  Rob Oct 31 '11 at 18:42
    
I'd probably log the exception in a logger and in turn throw another exception that my outer code could catch. Your outer code shouldn't need to know that the stock mapper is internally using a DB or PDO as it's data storage/retrieval. –  Zak Oct 31 '11 at 19:04
    
The one other thing I would probably do is create a "Stock" class that my mapper could populate with the data array from the database. Then I get a first class object instead of a PHP array as a return value. –  Zak Oct 31 '11 at 19:05
add comment
up vote 0 down vote

Two issues:

1) I would place the code to open the connection in the constructor.

2) You want to look into parametrized prepared statements.

You want to send a query like:

SELECT * from stock WHERE id = ?

Then send an array of parmeteters:

array(302)

This will prevent SQL injection.

share|improve this answer
    
I see the logic in there, but how do you deal with the different query's you need? like: SELECT stock.size people.age from sizes LEFT JOIN people ON stock.id = people.id? –  Rob Oct 31 '11 at 18:19
    
Well you can have queries as complex as you wish but the user parameters will ultimately become ?'s. You can also create a query builder where you can say $test->leftjoin(table) and build up SQL. This will slow you down a bit though if the server is getting a lot of hits. –  Len Oct 31 '11 at 18:26
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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