Skip to content
Pvtw

Ugly code

I bet you have written code before where you think: "What a ugly code but it works...". I do! So let's fix that, together. With 3 steps on how to fix a common problem: Nested if statements.

Before we get to that. Let's look at the following. I have a controller responsable for 'login with github' by using the first-party laravel package: Socialite.

So this is the store method how it looked before:

public function store(): RedirectResponse
{
    $githubUser = Socialite::driver('github')->user();

    $user = User::firstOrCreate([
        'github_id' => $githubUser->getId(),
    ], [
        'name' => $githubUser->getName(),
        'username' => $githubUser->getNickname(),
        'email' => $githubUser->getEmail(),
    ]);

    Auth::login($user);

    return redirect()->route('home');
}

Very ugly isn't it? Not only that, it has a big caveat. That is what if you have created a account throught the register page (not active right now) and then login with github, getting in the store method above? The code tells us that it is first searching for a user with the github id and if it doesn't exist, create a account. But the email address already exist in the database so you get a mysql error because the email column has a unique index.

Basically I have to rework the method to fix the issue. After some trial and error I ended up with this "beautiful" code:

public function store(): RedirectResponse
{
    $githubUser = Socialite::driver('github')->user();

    /** @var User|null $user */
    $user = User::where('github_id', $githubUser->getId())->first();

    if ( ! $user) {
        /** @var User|null $user */
        $user = User::where('email', $githubUser->getEmail())->first();

        if ( ! $user) {
            $attributes = [
                'name' => $githubUser->getName(),
                'email' => $githubUser->getEmail(),
                'github_id' => $githubUser->getId(),
            ];

            if (0 === User::where('username', $githubUser->getNickname())->count()) {
                $attributes = [
                    'username' => $githubUser->getNickname(),
                    ...$attributes,
                ];
            }

            /** @var User $user */
            $user = User::create($attributes);
        } else {
            $user->update(['github_id' => $githubUser->getId()]);
        }
    }

    Auth::login($user);

    return redirect()->route('home');
}

It may looks oke or you're facepalming about this code. "Why did you (I) do that?". The first problem: Nested if statement. It is hard to see the happy flow of the code. The happy flow is actually the else statement of the second if. But that is not obvious. Another problem is the reset of the $user variable. It maybe makes sense but it looks ugly. We can fix those problems! Let's do it together.

The first check we have to do is checking if there is a user with that github id.

$user = User::where('github_id', $githubUser->getId())->first();

Now what would be the if statement? One quick tip to prevent nested if statements is to negate the check. So instead of checking if $user is null, we do the opposite. When $user is not null. But we have a problem! We can not go out of the if statement by returning earlier. The solution is to extract to a private method where we can return the $user earlier. We can do it like this:

private function getUserFromGithubUser(SocialiteUser $githubUser): User
{
    /** @var User|null $user */
    $user = User::where('github_id', $githubUser->getId())->first();

    if (null !== $user) {
        return $user;
    }
}

This looks way cleaner. When $user is not null, we found the user with that github id and we can return the $user directly. Now we can do the next check. Finding a user by email. It is almost the same but with email instead of github id. And we do the same check. If $user is not null, return the user. But one more thing! We had that else statement where we update the user and set the github id. We have to include that now because we negated the if statement.

/** @var User|null $user */
$user = User::where('email', $githubUser->getEmail())->first();

if (null !== $user) {
    $user->update(['github_id' => $githubUser->getId()]);

    return $user;
}

Now we are done with the 'unhappy' flow. In the happy flow we are creating a new user to the database. We first set some attributes we need to create the user. According to the table design, the only attributes required to create a user, is the name and email address. But we are including the github id aswell. And if the username doesn't already exist, we also include the username. It looks like this:

$attributes = [
    'name' => $githubUser->getName(),
    'email' => $githubUser->getEmail(),
    'github_id' => $githubUser->getId(),
];

if (0 === User::where('username', $githubUser->getNickname())->count()) {
    $attributes = [
        'username' => $githubUser->getNickname(),
        ...$attributes,
    ];
}

return User::create($attributes);

A quick tip: You can merge two arrays easily with the spread operator.
So that looks fine. We return the created user directly because there is nothing else to do. It is the happy flow. For completeness, here is the full controller code:

<?php

declare(strict_types=1);

namespace App\Http\Controllers\Auth;

use App\Http\Controllers\Controller;
use App\Models\User;
use Illuminate\Http\RedirectResponse;
use Illuminate\Support\Facades\Auth;
use Laravel\Socialite\Contracts\User as SocialiteUser;
use Laravel\Socialite\Facades\Socialite;

final class LoginWithGitHubController extends Controller
{
    public function create(): RedirectResponse
    {
        return Socialite::driver('github')->redirect();
    }

    public function store(): RedirectResponse
    {
        $githubUser = Socialite::driver('github')->user();

        $user = $this->getUserFromGithubUser($githubUser);

        Auth::login($user);

        return redirect()->route('home');
    }

    private function getUserFromGithubUser(SocialiteUser $githubUser): User
    {
        /** @var User|null $user */
        $user = User::where('github_id', $githubUser->getId())->first();
        
        if (null !== $user) {
            return $user;
        }

        /** @var User|null $user */
        $user = User::where('email', $githubUser->getEmail())->first();

        if (null !== $user) {
            $user->update(['github_id', $githubUser->getId()]);

            return $user;
        }

        $attributes = [
            'name' => $githubUser->getName(),
            'email' => $githubUser->getEmail(),
            'github_id' => $githubUser->getId(),
        ];

        if (0 === User::where('username', $githubUser->getNickname())->count()) {
            $attributes = [
                'username' => $githubUser->getNickname(),
                ...$attributes,
            ];
        }

        return User::create($attributes);
    }
}

The action methods looks way cleaner now by introducing a private method where the magic happends. So I have a few tips on how to prevent nested if statements:

  • Negate the if check
  • Extract to method to allow early returns
  • Unhappy flow first then the happy flow

We actually still have a problem with reseting the user variable, but I haven't found a good solution for that other than renaming it.

Now this looks way cleaner. Hope this was helpful to you.