Explore our sites

Stack Exchange
tour help
Take the tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

Hey guyz i have created this model for codeigniter. Its working fine but i need to know if it can be more optimized. Sorry about my grammar english is not my native language.

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;
    }
}
share|improve this question
add comment

2 Answers

up vote 2 down vote accepted

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.
share|improve this answer
 
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.... –  Shayan Husaini Mar 2 '12 at 9:01
 
And i really feel bad that my reputation does not allow me to voteup your answer –  Shayan Husaini Mar 2 '12 at 9:03
 
1/ Printing the query is only because you're using the "development environment. Switch to production when needed. 2/ And you're right, the controller does the security for inputs. You can still use secure SQL queries. The model is most useful when a single method does an entire transaction, and hides annoying details from the controller. (3/ Oh and you can't upvote but you can accept my answer by clicking the tick below the upvote button). –  Quentin Pradet Mar 2 '12 at 9:22
 
Hmmmm.... thats great thnx @Cygal –  Shayan Husaini Mar 2 '12 at 9:58
 
you are wrong about the printing query when i changed the environment to production it still prints the entire query when an error occurs –  Shayan Husaini Mar 16 '12 at 9:45
show 1 more comment
up vote 1 down vote

the code 

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

in function insertOnDuplicateKeyUpdate

if you have sent values for some date field as SYSDATE() or NOW() wont execute the function. and will not insert proper value

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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