4
\$\begingroup\$

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;
    }
}
\$\endgroup\$
3
\$\begingroup\$

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.
\$\endgroup\$
  • \$\begingroup\$ Thanx for your valuable advices. I have used the security class of codeigniter to prevent from sql injection. what i believe model should only interact with the database it does not perform the validations and other security checks that are not related to database the only reason for using validation of parameter is that if parameters are not provided then codeigniter prints the complete query on browser which is definately a flaw. According to my opinion security checks must be done in controller. So please tell me the advantages of doing that in model instead of controller.... \$\endgroup\$ – Shayan Husaini Mar 2 '12 at 9:01
  • \$\begingroup\$ you are wrong about the printing query when i changed the environment to production it still prints the entire query when an error occurs \$\endgroup\$ – Shayan Husaini Mar 16 '12 at 9:45
  • \$\begingroup\$ Production mode hides PHP errors. Set db_debug to false to also hide database queries. \$\endgroup\$ – Quentin Pradet Mar 16 '12 at 10:10
1
\$\begingroup\$

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.

\$\endgroup\$

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.