3
\$\begingroup\$

I think I put too much code in my controller that was supposed to go in the model.

This is a part of my controller, I'm not gonna paste everything since there is a lot of code.

  public function ajaxUsers() {
    if($_GET["action"] == "listUsers") {

        if(!isset($_POST["search"])) {
            $this->_data['Records'] = $this->_model->getUsers();
            $this->_data['Result'] = "OK";
            $this->_data['TotalRecordCount'] = $this->_model->countUsers();
        }
        else {
            foreach($_POST['fields'][0] as $key => $post) {
                if ($post != "" && $key != "reg_date") {
                    $searchTerms = explode(' ', $post);
                    foreach ($searchTerms as $term) {
                        $term = trim($term);
                        if (!empty($term)) {
                            $like[] = $key." LIKE '%".trim($term, '\'')."%'";
                        }
                    }

                }
                else if ($post != "" && $key == "reg_date") {
                    foreach ($post[0] as $key2 => $date) {
                        $datetofrom = strtotime($date);
                        $datetofrom = date('Y-m-d', $datetofrom);
                        if ($date != "" && $key2 == "datefrom") {
                            $like[] = "DATE_FORMAT(".$key.", '%Y-%m-%d') >= '".$datetofrom."'";

                        }
                        if ($date != "" && $key2 == "dateto") {
                            $like[] = "DATE_FORMAT(".$key.", '%Y-%m-%d') <= '".$datetofrom."'";
                        }
                    }
                }
            }

            ($like) ? $where_clause = "WHERE ". implode(' AND ', $like) : $where_clause = "";   
            $this->_data['Records'] = $this->_model->filterUsers($where_clause);
            $this->_data['Result'] = "OK";
            $this->_data['TotalRecordCount'] = $this->_model->countfilterUsers($where_clause);
        }
        echo json_encode ($this->_data);
    }
}

And my model is mostly database queries:

  public function getUsers() {
    $data = $this->_db->select("SELECT * FROM ".PREFIX."users" . $this->_sort);
    return $data;
}

public function countUsers() {
    $data = $this->_db->select("SELECT count(*) as id FROM ".PREFIX."users");
    return $data[0]->id;
}

public function filterUsers($like_clause) {
    $data = $this->_db->select("SELECT * FROM ".PREFIX."users " .$like_clause. $this->_sort); 
    return $data;
}
public function countFilterUsers($like_clause) {
    $data = $this->_db->select("SELECT count(*) as id FROM ".PREFIX."users ".$like_clause);
    return $data[0]->id;
} 

Should I moved the foreach loops in the model?

\$\endgroup\$
1
  • \$\begingroup\$ I suggest to look at this lecture by Robert C Martin - Clean Architecture and Design - he uses slightly different naming, but focuses on application model, use case architecture and how its collaborators - View/Controller/data(base) should be plugged into it. App Model he presents is MVC Model but separated from internal data. That's what makes whole model in your code is just a boundry of application and your controller is closer to that application model than actuall controller. \$\endgroup\$
    – shudder
    Apr 11, 2015 at 9:57

1 Answer 1

1
\$\begingroup\$

Should I moved the foreach loops in the model?

Yes, definitely.

Currently, you have SQL in the model and the controller, which isn't a good idea. It makes it hard to make out what the actual query will be, and splitting up one functionality (database lookup in this case) in multiple classes is not a good idea in general. SQL should be part of the user model (or DAO), so create a method called searchBy($keywordsArray) there and use that.

Security

You are completely open to SQL injection, as you put $_POST values directly into your query as column name and like clause. When putting variable content into queries, you should always use prepared statements to defend against SQL injection if possible.

When it's not possible, for example with variable column names, then you should prepare a whitelist of your columns, and check if the user input is in it.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the feeback, so I can't use prepared statements in my example and have to implement whilelist for the comlun name? \$\endgroup\$
    – Speedwheel
    Apr 10, 2015 at 11:31
  • \$\begingroup\$ @Speedwheel right, for the column name you need a whitelist, and for the actual like keywords you need prepared statements (here's an example) \$\endgroup\$
    – tim
    Apr 10, 2015 at 12:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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