Applying MVC to a validation application

Question

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_roles' => $it_roles, 'tc_roles' => $tc_roles, 'assessor_roles' => $assessor_roles);

        $data['content'] = 'admin/create_user';

            parent::error_alert();
            $this->load->view($this->_at, $data);

        } else {
        $username = $this->input->post('username');
        $salt = $bcrypt->getSalt();
        $hashed_password = $bcrypt->hash($this->input->post('password'), $salt);
        $fname = $this->input->post('firstname');
        $mname = $this->input->post('middlename');
        $lname = $this->input->post('lastname');
        $department = $this->input->post('department');
        $role = $this->input->post('role');

            $user_login     = array($username, $hashed_password, $salt);
            $user_profile   = array($fname, $mname, $lname);
        $this->user_model->register_user($user_login, $user_profile, $department, $role);

        $data['content'] = 'admin/view_user';

            parent::success_alert(4, 'User Sucessfully Registered!', 'You may now login using your account');


        $data['data'] = array('username' => $username, 'fname' => $fname, 'mname' => $mname, 'lname' => $lname, 'department' => $department, 'role' => $role);
        $this->load->view($this->_at, $data);
    }

}

Views - this is where I put HTML, CSS, and JavaScript code (form validation code for the current form, looping through the data supplied by controller, a few if statements to hide and show things depending on the data supplied by the controller).

<!--User registration form-->
<form class="well min-form" method="post">
    <div class="form-heading">
        <h3>User Registration</h3>
    </div>

    <label for="username">Username</label>
    <input type="text" id="username" name="username" class="span3" autofocus>

    <label for="password">Password</label>
    <input type="password" id="password" name="password" class="span3">

    <label for="firstname">First name</label>
    <input type="text" id="firstname" name="firstname" class="span3">

    <label for="middlename">Middle name</label>
    <input type="text" id="middlename" name="middlename" class="span3">

    <label for="lastname">Last name</label>
    <input type="text" id="lastname" name="lastname" class="span3">

    <label for="department">Department</label>
    <input type="text" id="department" name="department" class="span3" list="list_departments">

    <datalist id="list_departments">
        <?php foreach ($data['departments'] as $row) { ?>
            <option data-id="<?php echo $row['department_id']; ?>" value="<?php echo $row['department']; ?>"><?php echo $row['department']; ?></option>
        <?php } ?>
    </datalist>

    <label for="role">Role</label>
    <input type="text" id="role" name="role" class="span3" list="">

    <datalist id="list_it">
        <?php foreach ($data['it_roles'] as $row) { ?>
            <option data-id="<?php echo $row['role_id']; ?>" value="<?php echo $row['role']; ?>"><?php echo $row['role']; ?></option>
        <?php } ?>
    </datalist>

    <datalist id="list_collection">
        <?php foreach ($data['tc_roles'] as $row) { ?>
            <option data-id="<?php echo $row['role_id']; ?>" value="<?php echo $row['role']; ?>"><?php echo $row['role']; ?></option>
        <?php } ?>  
    </datalist>

    <datalist id="list_assessor">
        <?php foreach ($data['assessor_roles'] as $row) { ?>
            <option data-id="<?php echo $row['role_id']; ?>" value="<?php echo $row['role']; ?>"><?php echo $row['role']; ?></option>
        <?php } ?>  
    </datalist>

    <p>
        <button type="submit" class="btn btn-success">Create User</button>
    </p>
</form>

<script>
    var departments = [];
    var roles = [];

    $('#list_departments option').each(function(i){
        departments[i] = $(this).val();
    });

    $('#list_it option').each(function(i){
        roles[roles.length + 1] = $(this).val();
    });

    $('#list_collection option').each(function(i){
        roles[roles.length + 1] = $(this).val();
    });

    $('#list_assessor option').each(function(i){
        roles[roles.length + 1] = $(this).val();
    });

    $('#department').blur(function(){

        var department = $.trim($(this).val()); 
        $('#role').attr('list', 'list_' + department);
    });

    var password = new LiveValidation('password');
    password.add(Validate.Presence);
    password.add(Validate.Length, {minimum: 10});
    $('input[type=text]').each(function(i){
        var field_id = $(this).attr('id');
        var field = new LiveValidation(field_id);
        field.add(Validate.Presence);
        if(field_id == 'department'){
            field.add(Validate.Inclusion, {within : departments});
        }
        else if(field_id == 'role'){
            field.add(Validate.Inclusion, {within : roles})
        }
    });
</script>

The code above is actually code from the application that I'm currently working on.

I'm also looking for some guidelines in writing MVC code, such as the things that should and shouldn't be included in views, models and controllers. How else can I improve the current code that I have right now?

I've written some really terrible code before (duplication of logic, etc.), which is why I want to improve my code so that I can easily maintain it in the future.

Answer 1

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.

<?php foreach($departments as $row) : ?>
<?php endforeach; ?>

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 { }
<span class="span3">
    <label></label>
    <input />
</span>

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!