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.