1
\$\begingroup\$

This class will take just the table name and data array and will do the rest, update or insert. I know it's not perfect, but I could use some feedback.

<?php
class DataBaseAction{

    private $tableName      ;
    public  $tableId    ;
    private $fieldNames = array() ;
    private $fieldType  = array() ;
    private $valueArray = array();


    //connect to database properties
    public $host       = "";
    public $dbName     = "";
    public $dbUsername = "";
    public $dbPassword = "";
    public $conection ;


    function establishConnection(){
        $con = mysqli_connect($this->host, $this->dbUsername, $this->dbPassword, $this->dbName ) ;
        if(!$con)
            die("error connecting to database ");
            $this->conection = $con;

    }
    public function setValuesArray($array){
        $this->valueArray = $array;
    }

// FIRST WE GOING TO FETCH FEILD NAMES AND STORE THEM INTO AN ARRAY 
    private function fetchFeildNames(){
        if(!isset($this->tableName) && $this->tableName == null)
            exit("you must provide table name");
       $query = mysqli_query($this->conection ,  "SELECT * FROM $this->tableName ") ;
       if($query === false){
           exit( "Error in fetching tables fields info");
       }
       while ($fieldInfo = mysqli_fetch_field($query)) {
            $feildNameArray[] = $fieldInfo->name;
            $feildTypeArray[] = $fieldInfo->type;   
        }
        $this->fieldNames = $feildNameArray;
        $this->fieldType  = $feildTypeArray;

    }

    public function setTableName($name){
        $this->tableName = $name ;
    }
    public function insert(){
       $this->fetchFeildNames();
       //changing the field names from array to string to use it in the query 

       $fieldNames = implode(",", $this->fieldNames);
       //prepearinf the question 
       $values="";
       for($i=0;$i<count($this->fieldNames);$i++){
            $values .= "?,";
        }
       $values = substr( $values , 0, -1);
       //prepearing the statment               
        $stmt = mysqli_prepare($this->conection,"INSERT INTO $this->tableName ($fieldNames) VALUES ($values)");
       if($stmt === false) {
            echo "ERROR :'(";
            exit;
        }

        //prepearing binding results;
        $typs="";
        for($i=0;$i<count($this->fieldType);$i++){
            switch ($this->fieldType[$i]) {
                case 3:
                  $typs .="i";
                  break;
                case 253:
                  $typs .="s";
                   break;
                case 252:
                  $typs .="s";
                    break;
            }
        }
        $finval = array ();
        $finval[]=  $typs;
        for($i=0;$i<count($this->valueArray);$i++){
            $finval[] = & $this->valueArray[$i];
        }
        call_user_func_array(array($stmt, 'bind_param'),$finval);
        mysqli_stmt_execute($stmt);
    }
    //function below to update table in database 
    public function update(){
        $this->fetchFeildNames();
        $text = "";
        for($i=1;$i<count($this->fieldNames);$i++){
            $text .= "`".$this->fieldNames[$i]."` = ? ,";
        }
        $text = substr($text, 0 ,-1);
        $sql="UPDATE $this->tableName SET $text WHERE $this->tableName.`id` = ? ";

        $stmt = mysqli_prepare($this->conection , $sql );

         //prepearing binding results;
        $typs="";
        for($i=1;$i<count($this->fieldType);$i++){
            switch ($this->fieldType[$i]) {
                case 3:
                  $typs .="i";
                  break;
                case 253:
                  $typs .="s";
                   break;
                case 252:
                  $typs .="s";
                    break;
            }
        }
        $typs .="i";

        $finval = array ();
        $finval[]=  $typs;
        for($i=0;$i<count($this->valueArray);$i++){
            $finval[] = & $this->valueArray[$i];
        }

        $finval[] = & $this->tableId;

        call_user_func_array(array($stmt, 'bind_param') , $finval );
        mysqli_stmt_execute($stmt);
    }

}
\$\endgroup\$
  • \$\begingroup\$ Make your database properties private instead of public. They should never be needed outside of your class. You've made an assumption that a table will have a column named ID and that that column will be the "key" when doing an update. Not much flexibility there. \$\endgroup\$ – Dave Aug 15 '16 at 13:12
  • \$\begingroup\$ ya i added the id as separate input to push it at the end later . i can put it with the input array then ill shift it to the end later . \$\endgroup\$ – yasir ALQAISI Aug 15 '16 at 16:31
2
\$\begingroup\$

I honestly don't see what value you are getting from this class. It looks like you are trying to build an object relational mapper and a database connection management class without really doing either successfully.

I don't understand the desire to make this overarching class that can perform all database insert and update actions. You are making your code much more complex and obfuscated by trying to have one class that can interact with different records representing different objects (i.e. database tables) in your systems. For example, what if you changed the schema on one of your tables to or remove a column. Rather than making a similar change in a single class associated with that table, you now have to search all around your codebase looking for all places where this more general class is used in order to determine if that particular usage is impacted by the table change. This is very much a concern for application maintenance.

You also have a very simply use case here. What happens when you need to do things like insert timestamps on your table records (a very common operation) and deal with more field types than the ones you have considered here? You well continue to have to branch you code to handle more and more edge cases, adding complexity.

There is absolutely nothing wrong with having specific database model implementations for each table/object type in your application. This makes it much clearer to the developer as to what the expected fields and behaviors are for each individual object, as opposed to them having to go review a database schema to understand what fields are available.

I would suggest an alternate approach to the object relational mapping functionality you are trying to achieve. You might start with an abstract DBModel class that can be inherited by individual classes specific to each table. This abstract class can require that each inheriting class implement their own logic for CRUD operations. Perhaps that abstract class looks something like this.

abstract class DBModel
{
    /*
    Define property to store valid mysqli object
    */
    protected $mysqli;

    /*
    Define abstract methods to force implementation by inheriting classes.
    */
    abstract public function select();
    abstract public function insert();
    abstract public function update();
    abstract public function delete();

    /*
    Define standard constructor. Here we use dependency injection to pass
    a valid mysqli object to the class that it can use for DB operations.
    This free the class from needing to know how to set up this dependency.
    */
    public function __construct(mysqli $mysqli) {
        $this->mysqli = $mysqli;
    }
}

You then extend this class for each for each DB table that you want to work with as an object. A sample implementation might look like:

class ObjectA extends DBModel
{
    /*
    Define properties applicable to this object
    */
    protected $id;
    public $field1;
    public $field2;
    public $field3;
    ...

    public function select($id) {
        /*
        Validate we have proper positive integer to correspond to the
        autoincrement value in the database.
        */
        if (empty($id) || !is_int($id) || $id < 1) {
            throw new InvalidArgumentException(
                'A positive integer value must be passed to select().'
            );
        }

        $query = '
            SELECT
                `field1`, `field2`, `field3` /* etc. */
            FROM `tableA`
            WHERE `id` = ?';
        $stmt = $this->mysqli->prepare($query);
        if(!$stmt) {
            // not shown perhaps throw exception here. At a minimum break execution
        }
        $stmt->bind_param('i', $id);
        $result = $stmt->execute();
        if(!$result) {
            // not shown - perhaps throw exception here. At a minimum break execution.
        }
        $stmt->store_result();
        if($stmt->num_rows() === 0) {
            // no match for that id. Perhaps this is an error in your system
            // perhaps not.  You need to determine what to do here.
            // You probably need to stop executing code in this method.
            // example behavior shown below.
            error_log(
                'ID value passed to select() function did not match record in DB.'
            );
            return false;
        } else {
            // this is valid id you we set id property on object
            $this->id = $id;
            // saturate the object based on result set
            $stmt->bind_result($this->field1, $this->field2, $this->field3, /* etc. */);
            $stmt->fetch();
        }

        // return this object to allow for method chaining.
        return $this;
    }

    /*
    Note shown - implement other required methods
    */
}

A few notes on this code example:

  • I have use the approach of dependency injection to pass a valid mysqli object to this class upon instantiation. This decouple this class from needed to understand how to set up this dependency. It just needs to know that it has a valid mysqli object.
  • This code uses mysqli in an object-orientated fashion rather than a procedural fashion as you are currently doing. Why mix OOP and procedural if you don't need to?
  • The example shows using exceptions to signify error conditions rather than die() or exit(). The class should not control stopping of program execution. Just throw and exception or signify and error condition and let the caller, which presumably has more information on the program state, make the decisions on what to do about the exception/error.
  • I have added some basic validation around method parameters. Either by type hinting (as in the abstract class constructor) or by explicitly testing the input. You need your code to fail fast when it gets bad data that the section of code doesn't know how to deal with. You also don't want to report out errors to standard out (i.e. via die(), exit(), echo(), etc.) unless perhaps this is code being used on a command line basis. User-friendly error messaging should be determined up the call stack from this class. Currently you have no validation whatsoever in your class. This is very problematic. I also don't know how to achieve proper validation with a one-class fits all approach. As on one table perhaps you need to validate that the field data conforms to certain standards that field data on another table. Another advantage to having specific classes related to each table.
    • This approach totally eliminates the need for any select queries before executing insert/update queries. There is no reason whatsoever to have to query the database twice to do one of these operations.

Let me also give some more specific comments on your provided code. I have added my comments within multi-line comment blocks below.

<?php
class DataBaseAction{

    private $tableName      ;
    public  $tableId    ;
    private $fieldNames = array() ;
    private $fieldType  = array() ;
    private $valueArray = array();


/*
Regardless as to whether you use my approach or not, you should not hardcode
database connection credentials into a class.  Have this as part of application
configuration.
*/
    //connect to database properties
    public $host       = "";
    public $dbName     = "";
    public $dbUsername = "";
    public $dbPassword = "";
/*
Spelling?
*/
    public $conection ;


/*
If you don't take dependency injection approach, at least consider using
singleton pattern so as to not unnecessarily reconnect to database.
*/
    function establishConnection(){
/*
Use object-oriented mysqli. Typical throughout
*/
        $con = mysqli_connect($this->host, $this->dbUsername, $this->dbPassword, $this->dbName ) ;
        if(!$con)
/*
Don't cease program operation here or output errors.  Let caller handle this.
*/
            die("error connecting to database ");
            $this->conection = $con;

    }
/*
Add validation on parameter. Probably a combo of array type hinting and
validation within method to make sure array is not empty, had expected range
of values, etc.
*/
    public function setValuesArray($array){
        $this->valueArray = $array;
    }

// FIRST WE GOING TO FETCH FEILD NAMES AND STORE THEM INTO AN ARRAY 
/*
Field not Feild. Be consistant.
If table name is a dependency, why not require upon instantiation?
*/
    private function fetchFeildNames(){
        if(!isset($this->tableName) && $this->tableName == null)
            exit("you must provide table name");
/*
If you really want to inesist on dynamically getting field names, why not query
the mysql information schema to get the information on available columns for the
table and set it once for all operations against that table rather than doing a
select query every time?
*/
       $query = mysqli_query($this->conection ,  "SELECT * FROM $this->tableName ") ;
       if($query === false){
           exit( "Error in fetching tables fields info");
       }
       while ($fieldInfo = mysqli_fetch_field($query)) {
            $feildNameArray[] = $fieldInfo->name;
            $feildTypeArray[] = $fieldInfo->type;   
        }
        $this->fieldNames = $feildNameArray;
        $this->fieldType  = $feildTypeArray;

    }

/*
Validation?
*/
    public function setTableName($name){
        $this->tableName = $name ;
    }
/*
Why not pass array of data to be inserted here rather than requiring two
different method calls to get the same result?
*/
    public function insert(){
       $this->fetchFeildNames();
       //changing the field names from array to string to use it in the query 

       $fieldNames = implode(",", $this->fieldNames);
       //prepearinf the question 
       $values="";
       for($i=0;$i<count($this->fieldNames);$i++){
            $values .= "?,";
        }
       $values = substr( $values , 0, -1);
       //prepearing the statment               
        $stmt = mysqli_prepare($this->conection,"INSERT INTO $this->tableName ($fieldNames) VALUES ($values)");
       if($stmt === false) {
            echo "ERROR :'(";
            exit;
        }

        //prepearing binding results;
        $typs="";
/*
What about other field types? This is not very flexible
*/
        for($i=0;$i<count($this->fieldType);$i++){
            switch ($this->fieldType[$i]) {
                case 3:
                  $typs .="i";
                  break;
                case 253:
                  $typs .="s";
                   break;
                case 252:
                  $typs .="s";
                    break;
            }
        }
        $finval = array ();
        $finval[]=  $typs;
        for($i=0;$i<count($this->valueArray);$i++){
            $finval[] = & $this->valueArray[$i];
        }
        call_user_func_array(array($stmt, 'bind_param'),$finval);
        mysqli_stmt_execute($stmt);
    }
    //function below to update table in database 
    public function update(){
        $this->fetchFeildNames();
        $text = "";
        for($i=1;$i<count($this->fieldNames);$i++){
            $text .= "`".$this->fieldNames[$i]."` = ? ,";
        }
        $text = substr($text, 0 ,-1);
        $sql="UPDATE $this->tableName SET $text WHERE $this->tableName.`id` = ? ";

        $stmt = mysqli_prepare($this->conection , $sql );

         //prepearing binding results;
        $typs="";
/*
Why are you doing thiss twice?
*/
        for($i=1;$i<count($this->fieldType);$i++){
            switch ($this->fieldType[$i]) {
                case 3:
                  $typs .="i";
                  break;
                case 253:
                  $typs .="s";
                   break;
                case 252:
                  $typs .="s";
                    break;
            }
        }
        $typs .="i";

        $finval = array ();
        $finval[]=  $typs;
        for($i=0;$i<count($this->valueArray);$i++){
            $finval[] = & $this->valueArray[$i];
        }

        $finval[] = & $this->tableId;

        call_user_func_array(array($stmt, 'bind_param') , $finval );
        mysqli_stmt_execute($stmt);
    }

}
\$\endgroup\$
  • \$\begingroup\$ Thanks sir for implementing your time to help me understand php more ! my whole aim was and maybe I am wrong to make it complicated like that! is to make one class that require nothing but a table name and an array as an input the array should match the targeted table fields counts and types and if its not it give back error . more skilled programmer could do great job achieving this . the way you mentioned is new to me . you used a class let call it (A) that take MySQL object as a parameter and the class(A) have methods that represent basic database ..... ill finish in the other comment \$\endgroup\$ – yasir ALQAISI Aug 22 '16 at 19:16
  • \$\begingroup\$ functions and these are abstract methods to let us fill them depending on the object that will extends the class (A) . that mean if i have 4 tables then i will have 4 objects that extends the (A) class each one have inputs deepening on the table structure . \$\endgroup\$ – yasir ALQAISI Aug 22 '16 at 19:22
  • 1
    \$\begingroup\$ @yasirALQAISI That is correct. And to your comment about a more skilled programmer being able to do a great job at what you were attempting to do, I would argue that a more skilled programmer would take an approach similar to mine, or use one of the several good ORM and/or database interaction libraries that are available out there like Doctrine, Eloquent, or similar. \$\endgroup\$ – Mike Brant Aug 22 '16 at 20:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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