Is it bad practice to follow up a call to a terminating function with exit()?

Question

I'm currently working on a php frontend. Specifically, the authentication process.

I'll spare everyone the exact details of the decision tree, but it includes a lot of checks and about half of them branch to a call to do_logon() which generates the login page and then calls exit to stop processing further.

Currently, I follow up each of those calls with another exit statement in the branch itself:

if($db->authenticate($user,$password)) {
 ... continue processing checks...
} else {
 $message='Invalid password';
 do_logon($message,true);
 exit;
}

Despite knowing that that final exit will never actually be called because do_logon ends on an exit.

My reasoning is that while I know that do_logon will always exit, other people who may wind up working on this codebase may not, or if I'm asked to maintain this code half a decade from now I might not remember, so adding the superfluous exit statement doesn't alter the code flow but does make it clear that processing will stop there, but am I overlooking something?

• amon: In general, it would be preferable to have do_logon do one thing only, instead of generating the login page and sending it to the browser and exiting. I'd structure this as echo render_template("logon", [$message, true]); exit
• CodesInChaos: IMO it's already bad practice that do_logon calls exit. It implies that your code has bad structure.

Answer 1

No, you should not exit or die. You can either have the do_logon end the script internally (bad practice) or allow the script to finish on it's own.

If you place this code in a function, then you can just return from the function. Allowing all the calls to propagate until the script is finished.

function auth() {
 if(!$db->authenticate($user,$password)) {
 $message='Invalid password';
 do_logon($message,true);
 return;
 }
 ... continue processing checks...
}

If your current design restricts this kind of behavior, then it's preferred to throw an exception.

 if(!$db->authenticate($user,$password)) {
 $message='Invalid password';
 do_logon($message,true);
 throw new Exception("Unexpected return from do_logon");
 }
 ... continue processing checks...

You can catch the exception higher up the call chain, or allow the server to give a 500 response.

Why throw an exception?

Something has gone unexpectedly wrong inside do_logon. Maybe the code changed or there is a use case you didn't account for. Either way, the current request to the server is no longer valid. If something higher up can catch the exception and handle it, then fine otherwise stop the current request.

Answer 2

The general answer to your question is that you should Die() and not exit.

You are describing a place in your code that should never be reached and (regardless of what anyone thinks of this particular example) these situations do arise. The proper thing to do is to implement a Die() function (if you don't already have one) and call it wherever something "can't happen". I guarantee before too long one of these will be hit, regardless of "it can't happen"!

The Die() function should log the call, including where it was called from, and then terminate. Often it can be derived from an Assert() function, which tests that some condition is true and dies if not. If you're not familiar with these concepts, or are not already using them, a bit of reading might be in order.