Connecting to a database and running CRUD operations

Question

I have a database class that connects to a database and runs CRUD operations. I need a critique of how well this class adheres to SOLID principles. A class to run all database operations seems like 'Single Responsibility' to me. Is there any need for further abstraction, such as an interface of some kind?

<?PHP

class Database 
{

  private $pdo='';
  public $dbConn='';

  private $dbName='';
  private $dbHost='';
  private $dbUser='';
  private $dbPass='';


public function __construct($database)
{
  $this->dbName=$database;      
}

private function dbConnect()
{
  //place connection in try catch, so errors can be displayed or sent to log
    try
    {
    $this->pdo= new PDO('mysql:dbname='.$this->dbName.';host=xxx.xxx.xx.xxx','abcdef','vwxyz');
    $this->pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

    $this->dbConn=true;

    return true;

    } catch(PDOException $e){

      return $e;

    }

} //End dbConnect


public function connect(){

    if ($this->dbConnect()) {

         if ($this->dbConn) {

           $myPDO=$this->pdo;

           return $myPDO;
         }


    } else { return false; }

}


function tableExists()
{

   $findTable="SHOW TABLES FROM ".$this->dbName." WHERE tables_in_".$this->dbName." LIKE 'prod%' ";

   $query=$this->pdo->query($findTable);
   $tablesFound=$query->fetch();

   return $tablesFound;

} //END Function tableExists.


public function select($table,$rows='*', $where=null,$order=null,$limit=null)
{

  is_array($rows) ? $rows=implode(',', $rows) : $rows=$rows;

   $stmt='SELECT '.$rows.' FROM '.$table;

  if ( !is_null($where) ) 
    $stmt .=' WHERE '.$where;

   if (! is_null($order) )  
    $stmt .=' ORDER BY '.$order.' DESC ';

  if (! is_null($limit) )  
    $stmt .=' Limit '.$limit;

    //if table exists proceed with Query
    $query=$this->pdo->query($stmt);

    $data=$query->fetchAll(PDO::FETCH_ASSOC);

   return $data;

} //End Function Select


public function insert($table,$values,$rows=null)
{         
      $ins='INSERT INTO '.$table;

      if ($rows != null) {

        $ins .= ' ('.implode(',', $rows).')';
      }

      for ($i=0; $i<count($values);$i++ )
      {
         $bindingVals[]=':val'.$i; 
      }

        $bindVals=implode(',', $bindingVals);
        $ins .=' VALUES ('.$bindVals.') ';

        $params=array_combine($bindingVals, $values);
        $insert=$this->pdo->prepare($ins);

        foreach ($params as $key => &$value) {
          $insert->bindParam($key, $value,PDO::PARAM_STR);
         }


         try {
           $insert->execute();
         } catch (Exception $e) {
           $error=$e->getMessage();

           echo $error.'<br><br>' ;
         }

   }//END Function insert



public function setUpdateVals(array $vals)
{
  foreach ($vals as $k => $v) 

  {
    $values[]=$k.'='.$this->pdo->quote($v); 
  }

  $values=implode(',',$values);

  return $values;
}                        


public function update($table,$rows,$where=null)
{

  $sql='UPDATE '.$table.' SET '.$rows;
  if ($where != null) 
  {
    $sql.=' WHERE '.$where;   
  }

  $stmt=$this->pdo->prepare($sql);
  if ($stmt->execute()) {

    return true;
  }

}//END Function update


public function delete($table,$where=null)
{

     if ( is_null($where) ) 
     {

        if (confirmTableDelete()) 
        {
          $sql="DELETE $table";
        }

     } else{

      $sql='DELETE FROM '.$table.'WHERE'.$where;
    }

    $stmt=$this->pdo->prepare($sql);
    $stmt->bindParam(':Value',$Value,$Data_Options);
    $stmt->execute();

 }



}

Answer 1

I think the database class as you wrote it has no use. You write a database class to provide an easier interface with the database. Instead you split up queries into multiple parts, but are still somewhat writing sql. A good database class allows you to use it without having to worry about escaping data, or even if the query was executed (instead of loaded from a cache).

Whitespace

Take some time to fix the whitespace in your code. The whitespace around several operators is inconsistent, such as the spaces around the assignment operator, and the spaces after a comma.

The indentation of your code is incorrect. Make sure that all your code is consistently indented. This makes the code a lot more readable, and thus more maintainable.

Documentation

Your code lacks any documentation or comments. This makes it hard to understand and hard to use, because it is unclear what code can return at any time.

Methods and variables

connect(..)

I am not sure what you are trying to accomplish splitting your code between connect(..) and dbConnect(..). There are several issues with this code. If dbConnect(..) throws, you return the exception, which is not falsy. This means that the if-statement in connect(..) will never be false. This results in connect(..) returning either true or nothing at all.

If connect(..) is called multiple times, you simply connect multiple times instead of just ignoring future calls based on $dbConn.

$dbConn

Why is this variable public? The naming could be better, and you probably don't want external code to modify this variable. Instead provide access to it via a getter.

table_exists(..)

I am unsure why this function exists. The name suggests that it would take an argument with a table name, and would return true or false. Instead it gives an array with table names.

select(..)

This code is vulnerable to sql injection. I would expect a database class to sanitize anything you pass to it. Why would surrounding code do that for the Database class? Check each of the arguments against what you expect the arguments to contain.

PDO::query returns a PDOStatement or false. You do not check for false, resulting in warnings and errors in the following code.

PDOStatement::fetchAll returns false on failure. Since your code lacks documentation I can't determine if this is the intended behaviour, but usually when you build a Database class you do this to encapsulate the PDO interface and provide easier to use methods.

insert(..)

This code is vulnerable to sql injection. See the previous section.

This method prints an error message. I do not expect a database class to print anything! Either provide a different way of accessing the error if it is required or log it. Make the method return true or false based on if it succeeds.

Don't use a catch all catch block. Instead figure out what the code can throw, and act accordingly. According to the documentation, that method does not throw.

setUpdateVals(..)

This function should be removed. It is not the responsibility of other code to escape values. In any case, you should not use PDO::quote and instead use prepared queries.

update(..)

While an update query without a where clause is possible, it is extremely unlikely you need it. Instead, the chances are much greater that you instead forgot to pass a value or did not properly check if a variable existed, or a return value was set.

You probably are tired of hearing it, but the code in this method is vulnerable to sql injection. A prepared query without any placeholders could as well be a normal query. You know how to bind variables to a variable number of placeholders, as you used it before. Use it here.

This method either returns true or nothing at all. Instead just return $stmt->execute(). The same problem where you do not check if PDO::prepare returned a boolean exists here too.

delete(..)

confirmTableDelete(..) does not exist. In any case, it is not the responsibility of a database class to interact with the user. It provides an interface between the database and the rest of the code. Assume that when delete is called, it is intended to be used.

You bind to :Value, but no such placeholder exists. $Data_options does not exist either.

The method does two separate things. I think you should split it into two distinct methods, each doing exactly one thing. This prevents unfortunate mistakes with missing tables.