Codeigniter Model Optimization

Question

I have created this model for Codeigniter. It's working fine, but I need to know if it can be more optimized.

class Basic_Function_Model extends CI_Model 
{

    var $msg_invalid_array;
    var $msg_no_table;

    function __construct()
    {
        parent::__construct();
        $this->msg_invalid_array    =   "Data must be provided in the form of array";
        $this->msg_no_table         =   "Table does not exist in database";
        $this->load->database();
    }

    public function insertInDatabase($table, $insertData, $mode = "single")
    {
        if( !is_array($insertData) ) {
            return $this->msg_invalid_array;
        }

        if( !$this->validateTable($table) ) {
            return $this->msg_no_table; 
        }

        if( $mode == "batch" ) {
            return $this->db->insert_batch($table, $insertData);
        } else {
            $this->db->insert($table, $insertData);
            return $this->db->insert_id();
        }
    }

    public function updateInDatabase($table, $updateData, $conditionData)
    {
        if( !is_array($updateData) || !is_array($conditionData) ) {
            return $this->msg_invalid_array;
        }

        if( !$this->validateTable($table) ) {
            return $this->msg_no_table; 
        }

        if( $this->db->update($table, $updateData, $conditionData) )
            return $this->db->affected_rows();
        else
            return false;
    }

    public function deleteFromDatabase($table, $conditionData)
    {
        if( !is_array($conditionData) ) {
            return $this->msg_invalid_data;
        }

        if( !$this->validateTable($table) ) {
            return $this->msg_no_table; 
        }

        return $this->db->delete($table, $conditionData);
    }

    public function insertOnDuplicateKeyUpdate($table, $tableData)
    {
        if( !is_array($tableData) ) {
            return $this->msg_invalid_data;
        }

        if( !$this->validateTable($table) ) {
            return $this->msg_no_table; 
        }

        foreach( $tableData as $column => $value )
        {
            $columnNames[] = $column;
            $insertValues[] = "'".$value."'";
            $updateValues[] = $column." = '".$value."'";
        }
        $this->db->query("insert into $table(".implode(', ', $columnNames).") values(".implode(', ', $insertValues).") on duplicate key update ".implode(', ', $updateValues));
        return $this->db->insert_id();
    }


    private function validateTable($tableName)
    {
        $result = $this->db->list_tables();

        foreach( $result as $row ) {
            if( $row == $tableName )    return true;
        }
        return false;
    }
}

Answer 1

First of all, thanks for sharing your code, this is the best way to improve yourself!

The M in MVC

A Model in any MVC framework should provide an easy way to access a specific type of data. It allows you to stop worrying about database details and only access your specific type of data in a meaningful way. For a StackOverflow-like website, this means that your Controller can relax! Your controller doesn't want to know about INSERT INTO Questions(id, title, message, author) VALUES (9624, "Codeigniter Model Optimization", "...", "Shayan"). He just wants to do $questions->add("Codeigniter Model Optimization", "...", "Shayan") and know that all details (including security) are going to be handled by the Model.

The model should be in charge of ensuring everything goes well with the database, but that's not what you're doing here. You're creating a Leaky Abstraction, and this doesn't help you. Try coming up with a model that reduces the work the controller needs to do.

A few comments on the code itself.

• You're repeating yourself a lot at the start of each function. One fix would be to put this in a function: $checks = $this->check($table, $conditionData); if ($checks !== FALSE) return $checks;
• But the real fix is to remove those. $this->db->query() already removes FALSE when there's an issue. You should use it to check if there's an issue, and then decide what to do then. The handling of issues is what makes abstractions successful. Remember than "It's easier to ask for forgiveness than it is to get permission".
• Please use query bindings (at then end of the page) which will avoid SQL injections. They're using PHP's prepared statements, which you should learn about if you want to write secure applications.

Answer 2

The code

$insertValues[] = "'".$value."'";

in function insertOnDuplicateKeyUpdate.

If you have sent values for some date field as SYSDATE() or NOW(), it won't execute the function and will not insert the proper value.

Answer 3

Obviously much time has passed since the posting of this question, but I thought I would take a moment to add my review.

1. I wouldn't declare/use either of the strings that describe the query error relating to table name or datatype. If you have a script that is not using the correct table name or datatype, then you are going to find these occurrence during testing. If you have such flexible controllers that dynamic (or even user-supplied) table names are being passed in, then you need to validate the data before it gets to the model. Trash these:

    $this->msg_invalid_array    =   "Data must be provided in the form of array";
    $this->msg_no_table         =   "Table does not exist in database";
   

2. That said, if you are going to declare class properties, then obey the PSR-12 standard:

   Visibility MUST be declared on all properties.

   The var keyword MUST NOT be used to declare a property.

   ...but again just trash var $msg_invalid_array; and var $msg_no_table;

3. By removing the error strings that you originally return in the validating conditions, you standardize the return value's datatype as int on all "writing" queries. I do not recommend returning a mix of integer type data and booleans; I prefer to return (sometimes nullable) int types.

4. Because single row inserts have a fundamentally different data structure and return value versus bulk inserts, I don't see any value in trying to merge them into a single method. I recommend declaring separate methods, and declaring the datatypes.

    public function insertRowWrapper(string $table, array $data): int
    {    
        $this->db->insert($table, $insertData);
        return $this->db->insert_id(); 
    }
   
    public function insertBatchWrapper(string $table, array $data): int
        return $this->db->insert_batch($table, $insertData);  // returns affected_rows
    }
   

5. Again, remove the conditions and declare the data types in your update and delete methods. Different from the insert queries, the update and delete queries can execute without error and have 0 affected rows. For this reason, you should differentiate query results which truly affected rows and queries that made no changes to the database -- all subsequent processes will be far more accurate about the data used/presented.

    public function updateWrapper(string $table, array $setData, array $conditions): int
    {
        $this->db->update($table, $setData, $conditions);
        return $this->db->affected_rows();
    }
   
    public function deleteWrapper(string $table, array $conditions): int
    {
        $this->db->delete($table, $conditions);
        return $this->db->affected_rows();
    }
   

6. The only method that is providing some semblance of utility is the insertOnDuplicate() method, but it isn't implementing proper quoting on identifiers nor escaping the values. Of course, this method is employeeing driver-specific syntax, so it will not be instantly migratable to all possible drivers. If it works for what your current driver, okay; it's just that if your driver changes, then this method might need to change as well.

    public function insertElseUpdate(string $table, array $data): int
    {
        foreach ($tableData as $column => $value) {
            $insertColumns[] = $this->db->protect_identifiers($column);
            $insertValues[] = $value;
            $updateExpressions[] = $this->db->protect_identifiers($column) . ' = ?';
        }
        $insertPlaceholders = implode(', ', array_fill(0, count($insertValues), '?'));
   
        $sql = 'INSERT INTO ' . $this->db->protect_identifiers($table)
            . ' (' . implode(', ', $insertColumns) . ')'
            . ' VALUES (' . $insertPlaceholders . ')'
            . ' ON DUPLICATE KEY UPDATE ' . implode(', ', $updateExpressions)
        $this->db->query($sql, [...$insertValues, ...$insertValues]);
        return $this->db->insert_id();  // I honestly didn't test how this would behave in either scenario
    }
   

Ultimately, I don't see much benefit in writing this class of CRUD wrappers. I recommend writing classes that individualize the entities/objects of your application, so that specific tailoring can be done within the scope of the class.