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

I'm fairly new to mvc and almost finished my auth class I wanted to check that I'm using it all correctly any feedback would be welcome. I used the PIP framework to get started http://gilbitron.github.com/PIP/

here's the controller

<?php

class Auth extends Controller {

protected $username;
protected $password;
public $action;
private $user;


function index()
{
   $template = $this->loadView('login');
   $template->render();
}

function login()
{

   $this->username = $_POST['username'];
   $this->password = $_POST['password'];
   $this->action = $_POST['which_action'];

   $login = $this->loadModel('auth_model');
   // pass the user details
   if($this->action=="login") {

       $login = $login->do_login($this->username,encrypt_password($this->password));

   switch($login[0]->active) {
     case "0":
    $this->redirect('auth/login/disabled');
    break;
    case "1":
    $this->redirect('dashboard/');
    break;
    default:
    $this->redirect('auth/login/error');
    break;
   }

   }


   $template = $this->loadView('login');
   $notification = $this->loadHelper('notification_helper');
   // check if the user is active or not
   $url = $this->loadHelper('url_helper');

   switch($url->segment(3)) {
    // account disabled
    case "disabled":
    $template->set('login_result', $notification->show_notification(1)); 
    break;
    // wrong login details
    case "error":
    $template->set('login_result', $notification->show_notification(2));
    break;
    // password reset
    case "reset":
    $template->set('login_result', $notification->show_notification(3));
    break;
    case "reseterror":
    $template->set('login_result', $notification->show_notification(3));
    break;
    case "changed":
    $template->set('login_result', $notification->show_notification(6));
    break;

   }



   $template->render();
}

function resetPassword() {

    $this->username = $_POST['username'];
    $this->password = $_POST['password'];

    $login = $this->loadModel('auth_model');
    $notification = $this->loadHelper('notification_helper');
    $url = $this->loadHelper('url_helper');
    $template = $this->loadView('reset_password');

    // The form has been posted lets reset the password     
    if($url->segment(3)==1) {


    $login->reset_password($this->username,getrandomstring(9));
    $check_reset = $login->check_reset($this->username);
    $this->user = $check_reset[0];


    switch($this->user->password_reset) {
    case 1:
    // success
    $email = $this->loadHelper('mail_helper');

    $email->send_template(1,$this->user,array($this->user->first_name." ".$this->user->last_name => $this->user->email));
    // the email was sent
    if($email->status=="sent") {
        $this->redirect('auth/login/reset');
    } else {
    // something went wrong
        $this->redirect('auth/login/reseterror');
    }

    break;
    case 0:
    // failure
    if($this->user->active!=0) {
    $template->set('password_result', $notification->show_notification(4));
    } else {
    // account disabled
    $this->redirect('auth/login/disabled'); 
    }

    break;
   }
    }
    if($url->segment(3)=="confirm") {
    // first lets check this is a real request
    $user = $login->get_user($url->segment(4),$url->segment(5));

    if($user[0]->active==1) {

    $template = $this->loadView('change_password');
    //password change procedure
    if($this->action=="change") {
        // verify the new password matches
        if($_POST['password']==$_POST['password_confirm']) {

            //crypt the password
            $password = encrypt_password($this->password);

            //update the account
            $login->change_password($url->segment(4),$password);

            //redirect to login
            $this->redirect('auth/login/changed');

            // Receive an email
            //$email->send_template(1,$check_reset[0]);


        } else {
        // return an error
        $template->set('password_result', $notification->show_notification(7));
        }
    }


    } else {
    // kick them back to the login screen
    $this->redirect('auth/login/reseterror');
    }

    }

    $template->render();
    // 2. Receive an email with a link to reset password


}


}

?>

and model

    <?php

class Auth_model extends Model {

public function do_login($username,$password)
{

    $q_params = array('username' => $username,'password' => $password);
    $result = $this->query("SELECT * FROM a_users where user_name = :username AND password = :password",$q_params);
    return $result;
}


public function reset_password($username,$seed)
{


    $q_params = array('username' => $username,'active' => '1','reset' => '1','reset_key' => $seed);
    $result = $this->query_update("UPDATE a_users SET password_reset = :reset,password_reset_key = :reset_key where user_name = :username AND active = :active",$q_params);
    return $result;

}
public function check_reset($username)
{

    $q_params = array('username' => $username,'active' => '1');
    $result = $this->query("SELECT * FROM a_users where user_name = :username AND active = :active",$q_params);
    return $result;

}
public function get_user($user_id,$random)
{

    $q_params = array('password_reset_key' => $random,'user_id' => $user_id, 'active' => '1');
    $result = $this->query("SELECT * FROM a_users where user_id = :user_id AND password_reset_key = :password_reset_key AND active = :active",$q_params);
    return $result;

}
public function change_password($user_id,$password)
{


    $q_params = array('password_reset' => 0,'password' => $password,'user_id' => $user_id, 'active' => '1');
    $result = $this->query_update("UPDATE a_users SET password = :password, password_reset = :password_reset where user_id = :user_id AND active = :active",$q_params);
    return $result;

}

}

?>
share|improve this question
add comment

1 Answer

From an MVC perspective, your controllers are doing far too much. Anything that's related processing on domain concepts (users, logins, products, etc) belongs in the model.

The entire point of MVC is Separation of Concerns so that each component is reusable/replaceable.

To test Separation of Concerns there are a few questions you can ask:

Are any of the components tightly coupled to the others?

In this case, both your models and views are very tightly coupled to your controllers. It's impossible for you to use a different view or model with that controller because the logic that chooses which view to use and which model to use exists within the controller itself. SoC would suggest separating these responsibilities.

If I replace a component will that involve repeating code?

If replacing a component requires rewriting lines of code this is a good indicator of poor Separation of Concerns. In your example, it's impossible to substitute the controller without repeating a lot of code. This limits flexibility and adds extra work. If you then update your view (which should be reusable) you then need to update every single controller that uses the view.

Controllers are not mediators between models and views. They take user input, update the model in some manner then the view requests relevant data from the model. See my answer here: Small MVC for PHP for a discussion about a very similar implementation.

For your implementation specifically logic in the form of "Display this if the user is logged in else display this" is by definition display logic. As such, it belongs in the view and not the controller.

The controller should not be telling the view to render. In fact, in a web MVC implementation, the controller doesn't even need to know of the view's existence (and in the implementations that do it's for a very specific reason). See the wikipedia diagram on MVC:

Model-view-controller

http://en.wikipedia.org/wiki/Model%E2%80%93view%E2%80%93controller

Which states: "A view requests from the model the information that it needs to generate an output representation."

share|improve this answer
    
Thank you for the detailed response that's really very helpful I don't think I fully understood the MVC approach and will do some more reading. It's hard to get my head around it. –  Joel Dec 18 '12 at 17:06
1  
A controller is the part of the software that gets in touch with the outside world. In case of a HTTP application, this is a HTTP request. The controller should only fetch the data from this request, pass it into the appropriate model and trigger what has to be done with it. Eventually some data comes back from the model that should go back to the user - via HTTP because that's what the controller knows. A good model allows to be used in different situations like "Desktop application needs login" - no HTTP, but user name and password, and "access allowed" or not as response. –  Sven Dec 19 '12 at 0:08
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.