HiveBrain v1.2.0
Get Started
← Back to all entries
patternphplaravelMinor

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

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
mvcandtryingmaintaincontrollermodelstructurecodeusersreview

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

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)

```
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

Solution

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

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

-
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();
}


-
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.

-
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());
    }
}


-
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)

Code Snippets

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

Context

StackExchange Code Review Q#40571, answer score: 4

Revisions (0)

No revisions yet.