patternphplaravelMinor
Laravel 4 clean code review of users controller and model, trying to maintain MVC structure
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
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
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
-
You could use a guard clause in
-
The fields in the User class are
-
In
-
I would create an explanatory local variable for the array for better readability:
(Clean Code by Robert C. Martin, G19: Use Explanatory Variables; Refactoring: Improving the Design of Existing Code by Martin Fowler, Introduce Explaining Variable)
-
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.