Laravel 4 clean code review of users controller and model, trying to maintain MVC structure

Posted on

Problem

I’d like you to review my attempt at creating an authorization class and let me know if there is re-factoring to be done and/or how I can make the code cleaner and abide by the MVC structure.

UsersController.php

<?php

    class UsersController extends BaseController {

    protected $user;

    public function __construct(User $user)
    {
        $this->beforeFilter('csrf', array('on' => 'post'));
        $this->user = $user;
    }

    public function getRegister() 
    {
        return View::make('admin.register');
    }

    public function postRegister()
    {
        try 
        {
            $this->user->insertUserIntoDatabase(Input::all());
        } 
        catch (ValidationError $e) 
        {
            return Redirect::to('admin/register')
            ->with('message', 'Something went wrong')
            ->withInput()   
            ->withErrors($e->getErrors());
        }

        return Redirect::to('admin/login')
                ->with('message', 'Thank you for creating a new account');
    }

    public function getLogin()
    {
        return View::make('admin.login');
    }

    public function postLogin()
    {
        if (Auth::attempt(array('email' => Input::get('email'), 'password' => Input::get('password') ))){
            return Redirect::to('admin/dashboard')->with('message', 'Thanks for logging in');
        }

        return Redirect::to('admin/login')->with('message', 'Your email/password combo failed.');
    }

    public function getLogout()
    {
        Auth::logout();
        return Redirect::to('admin/login')->with('message', 'You have been logged out.');
    }

    public function getDashboard()
    {
        return View::make('admin/dashboard');
    }   
}

Users.php (Model)

<?php

use IlluminateAuthUserInterface;
use IlluminateAuthRemindersRemindableInterface;

class User extends Eloquent implements UserInterface, RemindableInterface {

    /**
    * The database table used by the model.
    *
    * @var string
    */
    protected $table = 'users';

    /**
    * The attributes excluded from the model's JSON form.
    *
    * @var array
    */
    protected $hidden = array('password');

    protected $fillable = array('firstname', 'lastname', 'email');
    /**
    * Get the unique identifier for the user.
    *
    * @return mixed
    */
    public function getAuthIdentifier()
    {
        return $this->getKey();
    }

    /**
    * Get the password for the user.
    *
    * @return string
    */
    public function getAuthPassword()
    {
        return $this->password;
    }

    /**
    * Get the e-mail address where password reminders are sent.
    *
    * @return string
    */
    public function getReminderEmail()
    {
        return $this->email;
    }

    public function insertUserIntoDatabase($input)
    {
        $validation = new ServicesValidatorsUsers;

        if ($validation->passes()) {

            $user = new User;
            $user->firstname = Input::get('firstname');
            $user->lastname = Input::get('lastname');
            $user->email = Input::get('email');
            $user->password = Hash::make(Input::get('password'));
            $user->save();
        }

        else {
            $this->errors = $validation->errors;
            throw new ValidationError($validation->errors);
        }
    }
}

Solution

I didn’t used PHP in the last few years, so just some generic notes:

  1. The $input parameter of insertUserIntoDatabase($input) is unused. I guess you wanted to use the parameter instead of direct Input:: calls inside the function.

  2. You could use a guard clause in insertUserIntoDatabase to make the code flatten:

    public function insertUserIntoDatabase($input)
    {
        $validation = new ServicesValidatorsUsers;
    
        if (!$validation->passes()) {
            $this->errors = $validation->errors;
            throw new ValidationError($validation->errors);
        }
    
        $user = new User;
        $user->firstname = Input::get('firstname');
        $user->lastname = Input::get('lastname');
        $user->email = Input::get('email');
        $user->password = Hash::make(Input::get('password'));
        $user->save();
    }
    
  3. The fields in the User class are protected. You might want to use private instead. See: Should I always use the private access modifier for class fields?; Item 13 of Effective Java 2nd Edition: Minimize the accessibility of classes and members.

  4. In postRegister() you could move the return inside the try block. I guess Redirect::to doesn’t throw any ValidationError exceptions, so the following is the same:

    public function postRegister()
    {
        try 
        {
            $this->user->insertUserIntoDatabase(Input::all());
            return Redirect::to('admin/login')
                ->with('message', 'Thank you for creating a new account');
        } 
        catch (ValidationError $e) 
        {
            return Redirect::to('admin/register')
            ->with('message', 'Something went wrong')
            ->withInput()   
            ->withErrors($e->getErrors());
        }
    }
    
  5. if (Auth::attempt(array('email' => Input::get('email'), 'password' => Input::get('password') ))){
        return Redirect::to('admin/dashboard')->with('message', 'Thanks for logging in');
    }
    

    I would create an explanatory local variable for the array for better readability:

    $credentials = array(
        'email' => Input::get('email'), 
        'password' => Input::get('password') 
    );
    if (Auth::attempt($credentials)) {
        return Redirect::to('admin/dashboard')->with('message', 'Thanks for logging in');
    }
    

    (Clean Code by Robert C. Martin, G19: Use Explanatory Variables; Refactoring: Improving the Design of Existing Code by Martin Fowler, Introduce Explaining Variable)

Leave a Reply

Your email address will not be published. Required fields are marked *