4
\$\begingroup\$

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 Illuminate\Auth\UserInterface;
use Illuminate\Auth\Reminders\RemindableInterface;

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 Services\Validators\Users;

        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);
        }
    }
}
\$\endgroup\$
4
\$\begingroup\$

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 Services\Validators\Users;
    
        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)

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I think that your guard clause logic might be reversed. It appears to be saying, "If validation passes, then throw an error". Should it be, if(!$validation->passes()) ? \$\endgroup\$ – clone45 Dec 27 '14 at 23:46
  • \$\begingroup\$ @clone45: Yes, you're right, it should be that. Thanks, I've fixed it. \$\endgroup\$ – palacsint Dec 28 '14 at 0:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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