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);
}
}
