Applying MVC to a validation application

I've been doing MVC for a few months now using the CodeIgniter framework in PHP but I still don't know if I'm really doing things right.

What I currently do is:

Model - this is where I put database queries (select, insert, update, delete). Here's a sample from one of the models that I have:

function register_user($user_login, $user_profile, $department, $role) {

    $department_id = $this->get_department_id($department);
    $role_id = $this->get_role_id($role);

    array_push($user_login, $department_id, $role_id);

    $this->db->query("INSERT INTO tbl_users SET username=?, hashed_password=?, salt=?, department_id=?, role_id=?", $user_login);
    $user_id = $this->db->insert_id();

        array_push($user_profile, $user_id);

    $this->db->query("
            INSERT INTO tbl_userprofile SET firstname=?, 
            midname=?, lastname=?, user_id=?
        ", $user_profile);
}

Controller - talks to the model, calls up the methods in the model which queries the database, supplies the data which the views will display(success alerts, error alerts, data from database), inherits a parent controller which checks if user is logged in.

```
function create_user(){
$this->load->helper('encryption/Bcrypt');
$bcrypt = new Bcrypt(15);

$user_data = array(
'username' => 'Username', 'firstname' => 'Firstname',
'middlename' => 'Middlename', 'lastname' => 'Lastname',
'password' => 'Password', 'department' => 'Department',
'role' => 'Role'
);

foreach ($user_data as $key => $value) {
$this->form_validation->set_rules($key, $value, 'required|trim');
}

if ($this->form_validation->run() == FALSE) {
$departments = $this->user_model->list_departments();
$it_roles = $this->user_model->list_roles(1);
$tc_roles = $this->user_model->list_roles(2);
$assessor_roles = $this->user_model->list_roles(3);

$data['data'] = array('departments' => $departments, 'it_

Your understanding of MVC seems to be skewed. There are slightly different implementations of MVC, so one person's definition of a certain aspect of it might be slightly different than another's. I'll explain how basic MVC works, and why I think your implementation is not a true implementation of MVC. Also, I'm going to make some comments on your separation of code, or rather, lack of separation. Methods should be separated by functionality. In other words, they should do one thing to the exclusion of all others. Understanding this will make understanding MVC a little easier.

Models

As PeeHaa said, your explanation seems to be off. I've seen other people mistakenly think of their actual databases as their model without even knowing it. This appears to be what you are doing here. What you have is not what I would call a model, but rather a mini controller with too much information. As I mentioned before there are slightly different implementations of MVC, but as I just pointed out, this more closely resembles a controller. Usually, should probably read always, a model can be thought of as an interface to the database. The database can be anything from a text document, to a JSON database, to an XML file, to a MySQL database. The model has to allow the controller to add(), remove(), fetch(), etc..., depending on the circumstances and without knowledge of what type of database it is accessing.

Also, as I pointed out above with that list of methods, the method names in a model will more closely resemble an actual task you would perform on a database. register_user() is something I would expect to see on a controller. It implies that you are not just adding a user to the database, but validating, sending emails, etc....

Now, my first comments on your actual code. register_user() is doing too much. $department_id and $role_id should be defined and pushed into the array beforehand. These variables are static, not to be confused with the keyword static. In other words, they don't depend on values inside that method to change them. Their values are going to stay the same no matter what you do inside that method.

Controllers

As the model is an interface between the database and controller, so is the controller an interface between the model and view. And just as the view does not need to know how the controller is doing what it is, so does the controller not need to know how the model is doing what it is. In other words, controllers don't need MySQL queries. I only mention this because I mentioned that your model resembled a controller and did not want you to become confused. The model should handle the creation/deletion of entries, the controller should only be concerned about making the model do it and then telling the view what it has done.

According to your method name create_user(), your controller appears to be doing what your model needs. I would switch these names around to avoid confusion. Asides from the actual method name being odd, the functionality is spot on. I won't go into detail to avoid confusion, but another implementation, and one that I favor, adds another level of abstraction by adding a router/dispatcher between the controller and view to handle POST, GET, SESSION, COOKIE, and authentication. CodeIgniter does something very similar here, sans the authentication, with methods such as $this->input->post().

Again, your methods are doing too much. create_user() should only "create a user". Even if you change that to register_user(). It doesn't need to be concerned with encryption or form validation. These things need to be done, but in their own methods.

Views

Your views are fine as far as PHP goes. However, if you ever need to code a view for someone completely oblivious to PHP, you could make it a little easier for them to understand by abstracting initial arrays and using standard view loop/if/else format. Example:

$departments = $data[ 'departments' ];//performed in controller before view is included.

I think I would also use extract() within these loops to abstract from the array again, but that is definitely a matter of preference.

Unless you are importing PHP variables into your JS, your JS should really be external.

Just like in PHP, with CSS you can make referencing a certain field easier if you find you are doing it consistently. For instance. All those "span3" classed inputs can be better written as so.

//CSS: .span3 input { }

    
    

Or since it appears that all those inputs use the same class, you can just ignore the class altogether and set the default CSS for the input tag.

Good Luck!

StackExchange Code Review Q#13453, answer score: 3