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.

2 Answers
2

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.

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.

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.

\$\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 CaloveMar 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 RiezebeekMar 8 '16 at 18:28

\$\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 RiezebeekMar 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\$
– cFreedMar 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 RiezebeekMar 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\$
– cFreedMar 6 '16 at 14:27