MySQL PDO class

Question

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?

<?php
 /**
 * Description of MySql
 * @name MySQL PDO
 * @version 1.0
 * @author Yauri
 * 
 */
 class MySql {
 private $mPDO;
 public function __construct($dbHost,$dbName,$dbUser,$dbPass) {
 try {
 $this->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 array $data Specify array keys as database column name 
 * @return boolean
 */
 public function queryInsert($tableName, $data) {
 $dataString = $this->buildInsert($data);
 $query = "INSERT INTO {$tableName} {$dataString}";
 $exec = $this->mPDO->prepare($query);
 if($exec->execute(array_values($data))){
 return true;
 }
 else{
 return false;
 }
 }
 /**
 * Method for update
 * @param string $tableName
 * @param array $data Specify array keys as database column name 
 * @param array $where Specify array keys as database column name 
 */
 public function queryUpdate($tableName, $data, $where) {
 $update = $this->buildUpdate($data);
 $condition = $this->buildWhere($where);
 $query = "UPDATE ".$tableName." SET {$update} {$condition}";
 $exec = $this->mPDO->prepare($query);
 $paramVal = array_merge(array_values($data),array_values($where));
 $exec->execute($paramVal);
 if($exec->rowCount()){
 return true;
 }
 else {
 return false;
 }
 }
 /**
 * Method for delete
 * @param string $tableName
 * @param array $where You must specify the key as column name 
 */
 public function queryDelete($tableName, $where) {
 $condition = $this->buildWhere($where);
 $query = "DELETE FROM {$tableName} {$condition}";
 $exec = $this->mPDO->prepare($query);
 $paramVal = array_values($where);
 $exec->execute($paramVal);
 $count = $exec->rowCount();
 if($exec->rowCount()){
 return true;
 }
 else {
 return false;
 }
 }
 /**
 * Method for build a string for insert query
 * @param array $data You must specify the key as column name
 */
 private function buildInsert($data) {
 $length = count($data);
 $column = " (";
 $values = " VALUES (";
 foreach($data as $key => $val) {
 if($length != 1){
 $column .= $key.", ";
 $values .= "?, ";
 }
 else {
 $column .= $key;
 $values .= "?";
 }
 $length--;
 }
 $column .= ")";
 $values .= ")";
 return $column.$values;
 }
 /**
 * Method for build a string for update query
 * @param array $data You must specify the key as column name
 */
 private function buildUpdate($data){
 $length = count($data);
 $updateData = "";
 foreach($data as $key => $val){
 if($length!=1) {
 $updateData .= $key." = ? , ";
 }
 else{
 $updateData .= $key." = ?";
 }
 $length--;
 }
 return $updateData;
 }
 /**
 * Method for build a string for selected column
 * @param array $column
 * @return string 
 */
 private function buildColumn($column){
 $length = count($column);
 $selectedColumn = "";
 foreach($column as $val){
 if($length!=1) {
 $selectedColumn .= $val.", ";
 }
 else{
 $selectedColumn .= $val;
 }
 $length--;
 }
 return $selectedColumn;
 }
 /**
 * Method for build a string for query which using condition
 * @param array $where You must specify the key as column name 
 * @return string
 */
 private function buildWhere($where) {
 $length = count($where);
 $condition = " WHERE ";
 foreach($where as $key => $val){
 if($length!=1) {
 $condition .= $key." = ? AND ";
 }
 else {
 $condition .= $key." = ?";
 }
 $length--;
 }
 return $condition;
 }
 }
 ?>

• vstm: Btw. a little description of what you expect from the code review would not hurt.

Answer 1

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

• n00bi3: Fixed. Please take a look :)
• vstm: @n00bi3: okay, i've added two minor points.
• n00bi3: thanks for helping me to write a good code. I've wrote many codes but I'm aware it wasn't a good code. Now I learn to write the good one :D
• n00bi3: Oh yeah, how about the SQL injection? Does those codes good safe enough for common SQL injection technique? And what should I change to prevent more attacks?
• vstm: @n00bi3: Well the field values are escaped by PDO. The field and table names aren't escaped yet - this should be fixed for good security.