HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

MySQL PDO class

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
mysqlclasspdo

Problem

Please give me any comment about these codes. Does it enough to prevent SQL injection? What I have to do to make the code better?

```
mPDO = new PDO("mysql:host=$dbHost;dbname=$dbName", "$dbUser", "$dbPass");

//$this->mPDO->setAttribute(PDO::ATTR_ERRMODE,PDO::ERRMODE_EXCEPTION);
$this->mPDO->setAttribute(PDO::ATTR_ERRMODE,PDO::ERRMODE_WARNING);

}
catch(PDOException $e){
die($e->getMessage());
}
}

/**
* Method for executing query
* @param string $query
* @data array Used on queryUpdate method
* @return array Result of query
*/
public function query($query){

$exec = $this->mPDO->prepare($query);
if($data) $exec->execute($data);
else $exec->execute();
$result = $exec->fetchAll();
return $result;
}

/**
* Method for selecting data
* @param $table string
* @param $column array
* @return array Result of query
*/
public function querySelect($table, $column, $where=NULL, $limit=NULL){

if($column!="*"){
$column = $this->buildColumn($column);
}

if(isset($where)){
$condition = $this->BuildWhere($where);
$query = "SELECT {$column} FROM {$table} {$condition}";
}
else {
$query = "SELECT {$column} FROM {$table}";
}

if(isset($limit)){
$query .= " LIMIT {$limit}";
}

$exec = $this->mPDO->prepare($query);

if(isset($where)){
$exec->execute(array_values($where));
}
else{
$exec->execute();
}

return $exec->fetchAll();
}

/**
* Method for insert
* @param string $tableName
* @param ar

Solution


  • Your BuildInsert-method is the only one which uses mysql_real_escape_string. Why? Why not just use parametrized queries like in your "select", "update" and "delete" cases?



  • Your query method uses a variable $data which is not defined. Probably a missing parameter.



-
if($exec->execute()){
    return "Insert into database succeed.";
}
else{
    return "Insert into database failed.";
}


This is bad, don't return a string when a bool would suffice. What if you want to translate your application?

  • Sometimes you use method names beginning with a lower case like queryInsert and in other cases you start with an upper case like QueryUpdate. Be consistent.



  • $mQuery - this could be replaced by a local variable. Except you want to extend your class so you can fetch the last query. Otherwise: ditch it.



  • $mDbHost - not used, ditch it.



Update

-
This:

if($exec->execute(array_values($data))){
    return true;
}
else{
    return false;
}


can be written as:

return $exec->execute(array_values($data));


-
There's also a special case for update and delete which might return a count of affected rows. I would solve it like that:

if($exec->execute($paramVal)){
    return $exec->rowCount();
}
else {
    return false;
}


That way you can check if the query failed by using the !== or ===-operators e.g.:

$rowsDeleted = $yourpdo->queryDelete("posts", array("PostID" => 5));
// $rowsDeleted might be 0 if the post with id "5" does not exist so 
// check with ===
if($rowsDeleted === false) {
    echo "There was an error";
} else {
    echo "{$rowsDeleted} rows affected";
}


The wording for your documentation would be returns the number of rows affected or FALSE on error.

Code Snippets

if($exec->execute()){
    return "Insert into database succeed.";
}
else{
    return "Insert into database failed.";
}
if($exec->execute(array_values($data))){
    return true;
}
else{
    return false;
}
return $exec->execute(array_values($data));
if($exec->execute($paramVal)){
    return $exec->rowCount();
}
else {
    return false;
}
$rowsDeleted = $yourpdo->queryDelete("posts", array("PostID" => 5));
// $rowsDeleted might be 0 if the post with id "5" does not exist so 
// check with ===
if($rowsDeleted === false) {
    echo "There was an error";
} else {
    echo "{$rowsDeleted} rows affected";
}

Context

StackExchange Code Review Q#5262, answer score: 2

Revisions (0)

No revisions yet.