2
\$\begingroup\$

I have the following code in a Symfony site that validates a user's credentials and logs them in via the Session class:

/**
 * Validates an Affiliate
 */
public function validateUser($username, $password, $session, $securityContext) 
{
    $repo = $this->em->getRepository('JCMainBundle:Affiliate');
    $user = $repo->findOneBy(["username" => $username]);

    if (!is_object($user)) return false;

    $passwordValidated = password_verify($password, $user->getPassword());

    if (!$passwordValidated) return false;

    // Create token for user
    $token = new UsernamePasswordToken($user, null, "affiliate_dashbaord", ['ROLE_AFFILIATE']);
    $securityContext->setToken($token);
    $session->set('_security_affiliate_dashboard', serialize($token));

    return true;
}

As you can see, the function has very simple return values--just booleans. The function that calls this function looks at the return value of this function and determines whether or not to redirect the user to the affiliate dashboard.

How can I handle the return values better? I have two possible paths than can return false, but the calling function will have no idea what caused "false" to be returned.

In the past I've done something like this:

$response = ["success" => false, "message" => "error message here"];

I return the response with the success and a message. Using this design, I can return false AND return what made it return false. However, this seems a bit unorthodox for use on the backend and seems to fit more in a REST API.

I could use an exception, but the role of the function is to validate, so it wouldn't make sense to throw an exception if the user credentials are incorrect.

\$\endgroup\$
1
\$\begingroup\$

I have taken the 'returning an array with contextual information from a function call' route before and I can say that it is probably never the route you want to take. When a function tries to log in a user, then the inability to log in should be considered an exceptional case and is therefore Exception worthy.

Also, your function has the side effect of not only validating a user, which is what its name suggests, but also creating the session. This is not what you want to be doing. You want functions that perform a single task and are good at performing that specific task. Create a function that simply validates the user and throws an exception if the user does not pass. The exception part is key.

Consider the function verifyUserCredentials(). What do you expect from this function? You would probably expect it to take some user credentials, say a username or user object and password, and verify that they are valid. You could have gone the route of creating a boolean function like areUserCredentialsValid(), but exception throwing functions allow the funtion that is calling it to maintain better readability.

Good Example

Let me explain what I mean with the following example:

public function loginUser ($username, $password, $session, $securityContext) {
    try {
        $user = $this->getUserByCredentials($username, $password);

        $this->startLoginSession($user);
    } catch (UserVerificationException $e) {
        // Inform the user of username/password mismatch
    }
}

private function getUserByCredentials ($username, $password) {
    $repo = $this->em->getRepository("JCMainBundle:Affiliate");
    $user = $repo->findOneBy("username" => $username]);

    $this->verifyUserCredentials($user, $password);

    return $user;
}

private function verifyUserCredentials ($user, $password) {
    if (!is_object($user)) {
        throw new UserVerificationException("User does not exist");
    }

    if (!password_verify($password, $user->password) {
        throw new UserVerificationException("Passwords don't match");
    }
}

private function startLoginSession ($user) {
    $token = new UsernamePasswordToken($user, null, "affiliate_dashbaord", ['ROLE_AFFILIATE']);
    $securityContext->setToken($token);
    $session->set('_security_affiliate_dashboard', serialize($token));
}

The single function you had written has been split up into four functions, each performing a specific task. The main entry function loginUser() is now extremely easy to understand. It simply retrieves the user and starts a login session. Oh and apparantly a verification exception can happen. Within a few seconds you have enough information about this function to know what it does, without looking at the guts of it.

If you were to look at the functions that are being called you would see that each performs a single task without any side effects, only exceptions. Each of these individual functions is easy to read, understand and maintain, which is very important.

Bad Example

Now compare the code above to the code below:

public function loginUser ($username, $password, $session, $securityContext) {
    $result = $this->validateUser($username, $password, $session, $securityContext);
    if (!$result["success"]) {
        // Inform user of username/password mismatch
    }
}

public function validateUser ($username, $password, $session, $securityContext) {
    $repo = $this->em->getRepository('JCMainBundle:Affiliate');
    $user = $repo->findOneBy(["username" => $username]);

    if (!is_object($user)) return ["succes" => false, "message" => "User does not exist"];;

    $passwordValidated = password_verify($password, $user->getPassword());

    if (!$passwordValidated) return ["succes" => false, "message" => "Passwords don't match"];

    // Create token for user
    $token = new UsernamePasswordToken($user, null, "affiliate_dashbaord", ['ROLE_AFFILIATE']);
    $securityContext->setToken($token);
    $session->set('_security_affiliate_dashboard', serialize($token));

    return ["succes" => true, "message" => "We did it!"];;
}

What the hell is going on here? The main entry function is small, which is good. However, it has become unclear what exactly it is doing. The function name suggests it is logging in a user, but at closer inspection it seems to only be validating the user input. We should probably check that function out to see what is actually going on.

A bunch of stuff is happening. The user is retrieved from the repository. The result is being checked for validity and in the meantime some weird array construction is being created. Aah and there it is at the bottom, the session for the user is also created!

I hope this (not so) short example has given you some insight into why the flow of your code from one single responsibility function to another is important and why exceptions are the preferred way for dealing with these sorts of situations. The flow of your functions becomes much clearer when you are not constantly checking for things in the meantime, but instead have a catch block at the end of your flow.

\$\endgroup\$
  • \$\begingroup\$ While I really like your solution (and is probably going to be the one that I use as the best answer), I'm not sure if splitting every different "job" of a function into separate functions would be good for readability. As cFreed mentioned, breaking off a part of a function into its own function seems to only have value if it's reusable in other parts of the class. For example, your function getUserByCredentials() technically doesn't "get the user" as it doesn't return an object. Instead it validates, and throws exceptions on failure, which means those two functions should be in one. \$\endgroup\$ – Robert Calove Mar 8 '16 at 18:13
  • \$\begingroup\$ That is actually an error in my code. that function is supposed to have a return $user; statement at the end, which I forgot and just added in. I disagree that the only reason to create a function is reusability. Functions also create structure and enhance readability and maintainability of your code. There is a book about building maintainable software by SIG, that actually vouches for this as well. The book suggests only creating functions with a maximum of 15 lines. A link to the book: sig.eu/en/building-maintainable-software \$\endgroup\$ – Thijs Riezebeek Mar 8 '16 at 18:28
1
\$\begingroup\$

Note: the suggestion below is founded on a certain vision about readability, that may not be shared by everybody, as illustrated by the totally opposite approach proposed by @Thijs Riezebeek.

A pretty simple, light, and though efficient solution I frequently use in such cases is to merely:

  • Return the appropriate explainatory message when there is an error.
  • Do nothing otherwise.

First it makes your function as simple as possible (when successfull you don't even need to return anything).

Then from the app which invokes the function it's easy to use while being readable, assumed you change the function name for something like invalidUser(). So the app can work like this:

if ($errorMessage = invalidUser(...)) {
  // take any appropriate action, using $errorMessage
}
// otherwise merely continue
\$\endgroup\$
  • \$\begingroup\$ Although the approach works, I would not suggest it. The if obstructs the attention from the function that is being called as well as its intentions. Furthermore it is weird for a function to return a value that evaluates to true when the result is actually not correct. \$\endgroup\$ – Thijs Riezebeek Mar 6 '16 at 4:27
  • \$\begingroup\$ @ThijsRiezebeek Clearly we don't share the same philosophy, nor we feel the same about what is easily readable or not. I read you own answer and totally disagree. Especially because, for me, there are only two good reasons for creating a function rather doing the job in the flow: 1) obviously, when it'll be reusable; 2) when it encapsulates a big enough volume of statements to let the calling flow lighter, and so more easily readable. In the other hand, I'm somewhat offended you downvote my answer: remember that SO asks to exclude any opinionated position. For my part, I'll not downvote yours. \$\endgroup\$ – cFreed Mar 6 '16 at 13:21
  • \$\begingroup\$ Alright, that is a mistake on my part then. I will remove my down vote from your answer. I did not mean to offend in any way, just show that I did not agree with your answer. I was unaware that down voting is not the mechanism for that. Thanks for clearing this up. I will not go into further discussion in the comments about our different views, but I suggest you read Clean Code by Robert C. Martin. \$\endgroup\$ – Thijs Riezebeek Mar 6 '16 at 13:32
  • 1
    \$\begingroup\$ @ThijsRiezebeek You're totally right saying we can't go into further discussion about our different views: not only it'd also go against the not-opinionated way SO expects, but more over it'd lead to a huge, interminable exchange :). FYI about downvote: it's supposed to point erroneous, wot working, or merely meaningless proposed solutions. Last point: thanks for your fair attitude and your willing to remove your downvote. You probably noticed you can't do it till I edit my answer: I'll do it now, taking the opportunity, for objectivity, to point the opinionated aspect of different methods. \$\endgroup\$ – cFreed Mar 6 '16 at 14:27

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.