Controller for handling user registration

Question

I spent a lot of time doing Go Horse Extreme programming, but now I want to be a better person.

How can I make this method smaller, better, more OOP?

/**
 * @return \Illuminate\Http\RedirectResponse
 */
public function store()
{
    if (!User::is_valid(Input::all()))
    {
        Flash::error(User::$errors->first());

        return Redirect::back()->withInput();
    }

    $existing_user = User::where('email', Input::get('email'))->first();

    if (is_null($existing_user)) // The user does not exists.
    {
        // Prepare user data
        $user_input = Input::only(
            'firstName',
            'lastName',
            'email',
            'password');

        $confirmation_token = str_random(60);
        $user_data = array_merge($user_input, ['confirmation_token' => $confirmation_token]);

        // Store user
        $new_user = User::create($user_data);

        // Prepare email
        $data['name'] = Input::get('firstName') . ' ' . Input::get('lastName');
        $data['confirmation_token'] = $confirmation_token;
        $mail_subject = 'Confirmação de e-mail';

        // Send email
        Mail::send('emails.auth.email_confirmation', $data, function ($message) use ($mail_subject)
        {
            $message->to(Input::get('email'))->subject($mail_subject);
        });

        // Log the new user in
        Auth::login($new_user);

        $flash_message = sprintf(
            'Enviamos um email para <strong>%s</strong>. Confirme sua identidade',
            Input::get('email')
        );

        Flash::warning($flash_message);

        return Redirect::route('admin.dashboard');
    } else
    {
        if ($existing_user->is_active())
        {
            //Try to validate
            $stored = $existing_user->password;
            $typed = Input::get('password');
            $match = Hash::check($typed, $stored);

            if (!$match)
            {
                Flash::warning(
                    'Você já possui uma conta.
                    Faça login para continuar.'
                );

                return Redirect::route('login');
            } else
            {
                Auth::login($existing_user);
                Flash::message(
                    'Você já possui uma conta.
                    Bem vindo de volta!'
                );

                return Redirect::route('admin.dashboard');
            }
        } else
        {
            Auth::login($existing_user);

            $flash_message = sprintf(
                'Enviamos um email para <strong>%s</strong>. Confirme sua identidade [Reenviar]',
                $existing_user->email
            );

            Flash::warning($flash_message);

            return Redirect::route('admin.dashboard');
        }
    }
}

It just handles user registration. Everything seems to be working when I run my Behat tests, but I feel (deep in my soul) that things could be a lot better.

Answer 1

There are many ways you can refactor this method but since you are using Laravel things are pretty easy. Here is my attempt on refactoring (I'd rather call it re-writing) this method.

First of all, this controller is overloaded with so many responsibilities, a controllers sole job should only be to accept an HTTP Request, defer the execution to responsible object and deliver an HTTP Response, that is all a controller need to do. so lets refactor it first.

Defer execution to service class

I'd create a service class with name AccountService and inject it into AccountController (or whatever you call it in your case), then on AccountService I'd create a method named

createNewAcocunt($firstName, $lastName, $email, $password)

which will validate the data and create new account.

Take care of responsibilities

The else part of controller method store() defeats the purpose of REST thingy, I'd just issue a redirect to login page with an appropriate message instead of trying to log them in. all of login stuff and account activation status checking should be performed over login route so it clearly does not belongs to the account creator (or signup) route. with these 2 things in place the controller method store() would look like this

public function store()
{
    $details = Input::only('firstName', 'lastName', 'email', 'password');
    try {
        $user = $this->accountService->createNewAccount($details['firstName'], $details['lastName'], $details['email'], $details['password']);
        Auth::login($user->id);
        return Redirect::route('admin.dashboard');
    } catch (ValidationException $validationEx) {
        return Redirect::back()->withError($validationEx->getErrors()); // assuming you set errors on exception object before throwing it
    }
}

Now over to the AccountService class.
The method createNewAcocunt($firstName, $lastName, $email, $password), by its name, should create an account with data fed to it. In case there is an error it should throw an exception.

Take advantage of events as they happen

Now, you need to take care of responsibilities once again, do not overload this method with mail sending, user login or stuff like that, the only responsibility of this method is to create an account. To handle the mail sending and other tasks you need to perform after creating an account you can use laravel's EventDispatcher class or Event facade (Documentation here).
My method would look like this

public function createNewAcocunt($firstName, $lastName, $email, $password)
{
    $validation = $this->accountValidator->validate(
            compact('firstName', 'lastName', 'email', 'password')
    );

    if($validation->fails())
    {
        $up = new ValidationException('Invalid data for new account');

        // set error messages on exception to communicate them to upper layer
        $up->setErrors($validation->errors);
        throw $up;
    }

    $token = str_random(60)

    // if you use a repository then
    $user = $this->repository->createNewAccount($firstName, $lastName, $email, $password, $token);
    // or do it in whatever way you prefer

    // Now that the user is created fire suitable event
    Event::fire('account.created', [$user]);

    return $user;
}

Most of it has already sorted out, now you can catch the event account.created and perform whatever action you want.
My advice for sending out emails would be to queue emails instead of sending them in real time, this will decrease the response time since the codes will no longer poll for email to be dispatched

Mail::queue('emails.auth.email_confirmation', $data, function ($message) use ($mail_subject)
{
    $message->to(Input::get('email'))->subject($mail_subject);
});

read more about email queue here

and at last, you can customize the validation error message for email:unique rule and make that include a link to login page. check out the custom error message section over here. basically you can just pass in an associative array to Validator like this:

$messages = [
    'email.unique' => 'Looks like you already have an account, please <a href=' . route('login') . '>Log in here</a>'
];

$validation = Validator::make($data, $rules, $messages);
$validation->errors->get('email'); // should display the message as defined above

I hope this will give you at least an idea to get started with. I, for one, am really happy about your decision to go pro.