Jump to content
Larry Ullman's Book Forums

Exit() And Questions Related To Conditional'S Scope And Good Practice


Recommended Posts

I have a question related to the scope of a conditional and what would be the best practice. A few threads away some user asked what happens when updates a record and using the "mysqli_stmt_affected_rows" to check if the record was updated or not, as well as the update is not mandatory, will return "0" if the user do not change anything. Another member gave a book answer:

if (mysqli_affected_rows ($dbc) == 1) { // If it ran OK
 echo "Password Updated";
} elseif (mysqli_affected_rows ($dbc) === 0) { // use three equal signs so there is no confusion with a false return
 echo "Password unchanged because it was the same as on record";
} elseif (mysqli_affected_rows ($dbc) == -1) { // update query failed
 echo "Oopsie! System error.";
}

(I am using prepared statements, but the idea is the same). Now I would be tempted to say that I am interested to catch the error, so I would rather reduce the code to:

if (mysqli_affected_rows ($dbc) >= 0) {
  // updated or canceled
}
elseif {
  // system error
}

Or even better, I would prefer to eliminate as much as possible unnecessary accolades and do just the check for the error and leave the rest of the code flow naturally. If there is no error, it will execute, otherwise what is between the accolades will execute and will probably be an exit() so it will not spill outside the conditional.

if (mysqli_affected_rows ($dbc) < 0) {
  // do something when error occurs...
 exit();
}

I found it more readable this way and in OOP I would probably use try-catch in a quite similar manner I guess. I am wondering if this is a good practice or not as some may argue that it makes more sense to keep everything within the scope, in this case, the conditional statement.

 

The question may be well generalized. For example if the user is logged-in we check for a session value, like the email in your examples. Wouldn't be easier instead of something like

if(userAuthenticated){
// do what is supposed to do
}
else{
// throw error
}

to check for the error only.

if(! userAuthenticated){
// throw error
}
// do what is supposed to do

 

Now it probably works either way, what I am asking is what is the best practice under the circumstances?

 

A few moths ago I was asking a similar question regarding the validation and an user replied that changing the type of a variable is bad practice and it should be value or NULL, TRUE or FALSE. Now the code written at the beginning of your book use this practice of changing the type of a variable. You first declare it FALSE, than if it pass the validation you give the variable the value from the form. Later in the book, you are using an error_array() to catch the errors, than check if this is empty before moving forward. It works either way, but if it's agreed that it is bad practice to change the type of a variable than the second way of doing things is appropriate, even if the first one works (and I personally, using a ternary operator) I found it much easier to use, read, understand).

 

Related to the same issue, I noticed that some programmer open the database connection at the beginning of the script/page and perform cleaning, like mysqli_free_result($query) and mysqli_close($database) even bellow the tag. Something like:


mysqli_free_result($q);
mysqli_close($dbc);
?>

I can clearly see it is easier this way (probably they are using some automated method of generating pages), but I am wondering if it's correct (I guess not). If you insert data into the database, let's say, if it's a success you will redirect the user to the next page; if there is an error you redirect it to an error page (like the code from beginning). Either way the code will not get to the end of the page to perform the cleaning. This happen in some of your examples as well, even if you do not place it at the end of the page, but in the context of "success operation" branch of conditional. On short, most of your examples contain something similar to this:

if (mysqli_affected_rows ($dbc) == 1){
// do something
}
else {
// error
header("Location: error.php");
exit();
}

Here I have two questions: if the code executes the redirect, why it reads the next line with exit()?

Second question, why don't we treat the "error" part of the branch in the same way we treat the "success" one? Why we do not perform the cleaning before exit() statement as well? I assume closing the database connection and freeing the query are optional?

else {
// error
mysqli_free_result($q);
mysqli_close($dbc);
header("Location: error.php");
exit();
}

Link to comment
Share on other sites

First, let me suggest that you not ask 8 questions in one post. You'll get more, better answers if you keep your threads more specific.

 

I have a question related to the scope of a conditional and what would be the best practice....Or even better, I would prefer to eliminate as much as possible unnecessary accolades and do just the check for the error and leave the rest of the code flow naturally. If there is no error, it will execute, otherwise what is between the accolades will execute and will probably be an exit() so it will not spill outside the conditional.

if (mysqli_affected_rows ($dbc) < 0) {
  // do something when error occurs...
 exit();
}

I found it more readable this way and in OOP I would probably use try-catch in a quite similar manner I guess. I am wondering if this is a good practice or not as some may argue that it makes more sense to keep everything within the scope, in this case, the conditional statement.

 

Well, for starters, you want to watch using the word "scope" here, as "scope" is a specific programming thing that doesn't apply in this case. Second, you DEFINITELY don't want to use exit as you are here. That terminates that page. In most situations, that would mean that the rest of the HTML page would not be included, leaving an incomplete and ugly mess for the user.

 

The question may be well generalized. For example if the user is logged-in we check for a session value, like the email in your examples. Wouldn't be easier instead of something like

if(userAuthenticated){
// do what is supposed to do
}
else{
// throw error
}

to check for the error only.

if(! userAuthenticated){
// throw error
}
// do what is supposed to do

 

Now it probably works either way, what I am asking is what is the best practice under the circumstances?

 

Easier, yes, but better, no. I gather you're comfortable with OOP and OOP has the wonderful try...catch...finally, but that construct doesn't directly equate to if-else. It's a matter of how many possibles avenues might be taken. In your example, with only one IF, you add some action if an error occurs but do the same other actions every time. So you're not turning off any actions if an error occurs, which probably won't be right in terms of what the user sees. Successful query or not successful query are two different results that probably need two different responses, hence if-else. Successful query that changed a row vs. successful query that didn't change a row vs. unsuccessful query are three different results that probably need three different responses: if, else-if, else.

 

Shorter, more succinct code is not always better, and is often worse for what you're giving the end user.

 

A few moths ago I was asking a similar question regarding the validation and an user replied that changing the type of a variable is bad practice and it should be value or NULL, TRUE or FALSE. Now the code written at the beginning of your book use this practice of changing the type of a variable. You first declare it FALSE, than if it pass the validation you give the variable the value from the form. Later in the book, you are using an error_array() to catch the errors, than check if this is empty before moving forward. It works either way, but if it's agreed that it is bad practice to change the type of a variable than the second way of doing things is appropriate, even if the first one works (and I personally, using a ternary operator) I found it much easier to use, read, understand).

 

Some people don't think you should change a variable's type, but I don't have a problem with it when done consciously. I think it depends upon your background: users who come to PHP from, say, Java, probably think it's terrible to change a variable's type as it's not possible in Java. (Just as some people think you shouldn't split an infinitive in English because that's a rule that comes from Latin, a language in which it's impossible to split an infinitive.) With error handling in particular, I put forth a couple of approaches in the book. Choose whichever you like.

 

Related to the same issue, I noticed that some programmer open the database connection at the beginning of the script/page and perform cleaning, like mysqli_free_result($query) and mysqli_close($database) even bellow the tag. Something like:


mysqli_free_result($q);
mysqli_close($dbc);
?>

I can clearly see it is easier this way (probably they are using some automated method of generating pages), but I am wondering if it's correct (I guess not). If you insert data into the database, let's say, if it's a success you will redirect the user to the next page; if there is an error you redirect it to an error page (like the code from beginning). Either way the code will not get to the end of the page to perform the cleaning. This happen in some of your examples as well, even if you do not place it at the end of the page, but in the context of "success operation" branch of conditional.

 

Performing clean up or not is again a matter of taste, as PHP will perform clean up for you when a script terminates. It's probably more correct to formally perform the clean up, though. But you don't need to free the results after an INSERT, only after SELECT.

 

I would be highly surprised if there's code in my book that wouldn't ever be executed, though. If you can give a specific example, I can verify this.

 

On short, most of your examples contain something similar to this:

if (mysqli_affected_rows ($dbc) == 1){
// do something
}
else {
// error
header("Location: error.php");
exit();
}

Here I have two questions: if the code executes the redirect, why it reads the next line with exit()?

 

I don't have any code like that in my book, I can almost guarantee. I don't think I ever redirect to an error page and I would generally avoid that approach as it doesn't allow you to provide specific error information to the user. In any case, you SHOULD use exit after a header() redirect to stop the script from continuing to execute. I say this explicitly in the book. PHP will continue to run the rest of the script even if the browser has moved on. That's not a terrible thing, but is unnecessary.

 

Second question, why don't we treat the "error" part of the branch in the same way we treat the "success" one? Why we do not perform the cleaning before exit() statement as well? I assume closing the database connection and freeing the query are optional?

 

Again, I really don't think I have code like that in my book. If you could provide a specific example of code that confuses you, that'd be better. But closing the database connection and freeing the query results are optional.

Link to comment
Share on other sites

Sorry for asking so many question in one thread, I was feeling they are related and just followed my train of thoughts (which derailed apparently). I'll try to keep it simpler and cleaner next time. Thank you again for answering, even to so such a crazy thread.

 

Pag.338 the "check login" script has such logic:

 

if($check){
// ok
header("Location: $url");
exit();
}
else {
// not ok, throw error
mysqli_close($dbc);
}

 

If the data is entered in the database correctly I redirect the user to the next page or any other page such "user account". If an error is trowed than the administrator receive the email (as per your "connection" script) and the user will see a generic error message. Why (for simplicity) not sending the user to a generic "error" page. If the code stops at exit() it will not be executed further and I see no mess, but the generic "error" page. I understand that it is good practice to use if-elseif-else the way it was intended and not cutting corners. However, I see no problem in doing redirects and using exit().

 

The other question unanswered is this. In the code above you are closing the database for one branch of the if-else, but not for the other. Extrapolating I may thing to the first example with if-elseif-else where I have 3 branches (and I can easily imagine one with more elseif's so with more than 3 branches). My question is why not performing the cleaning for each branch (in the example from your book, why not closing the database for the other branch as well?)

 

If all this stuff is optional, than I think this is the reason why other programmers (not in your book) simply add a statement to close the databse after the last HTML tag - because it doesn't matter.


mysqli_close($dbc);
?>

 

If it matters, than I guess it should be used for each branch following the specific logic of that branch. Following your example why not:

 

if($check){
// ok
mysqli_close($dbc); // perform cleaning like free statement, close query and close db, here just closing the db
header("Location: $url");
exit();
}
else {
// not ok, throw error
mysqli_close($dbc);
}

Link to comment
Share on other sites

Pag.338 the "check login" script has such logic:

 

if($check){
// ok
header("Location: $url");
exit();
}
else {
// not ok, throw error
}
mysqli_close($dbc);

 

If the data is entered in the database correctly I redirect the user to the next page or any other page such "user account". If an error is trowed than the administrator receive the email (as per your "connection" script) and the user will see a generic error message. Why (for simplicity) not sending the user to a generic "error" page. If the code stops at exit() it will not be executed further and I see no mess, but the generic "error" page. I understand that it is good practice to use if-elseif-else the way it was intended and not cutting corners. However, I see no problem in doing redirects and using exit().

 

So a few things...

1) That script is login.php.

2) I edited your code to better reflect the script's code (you had the final } after the database close).

3) No errors are being thrown.

4) The administrator is not being emailed.

5) The user does not see a generic error message but a specific one.

 

The problems in this particular case with redirecting to a generic error page are multiple. First, a specific error will be shown by the script, so you'd lose that. Or you'd have to pass it along. Second, your solution ends up with an extra PHP script being executed, which is wasteful for both the client and the server. And, most importantly, third, in this particular case, the "error" is the user providing the wrong credentials. If you send them to a new page, then you're forcing the user to GO BACK and try again, as opposed to just presenting the form for the user to try again, with the specific error message above the form (or inline).

 

I'll ask you: what do you imagine the benefits to be of redirecting to a generic error page?

 

The other question unanswered is this. In the code above you are closing the database for one branch of the if-else, but not for the other. Extrapolating I may thing to the first example with if-elseif-else where I have 3 branches (and I can easily imagine one with more elseif's so with more than 3 branches). My question is why not performing the cleaning for each branch (in the example from your book, why not closing the database for the other branch as well?)

 

Because I made a mistake. The database connection should be closed before the redirect. It's technically not a problem, as the database connection will be closed regardless, but for the sake of consistency, I should have done that. Apologies for any confusion that may have caused.

 

If all this stuff is optional, than I think this is the reason why other programmers (not in your book) simply add a statement to close the databse after the last HTML tag - because it doesn't matter.

 

Well, we have to define "matters". There are things that matter in that if they're not done, things don't work right, bugs are introduced, etc. And there are things that matter in terms of style and consistency. In this particular case, my failure to close the database connection has no ill effect. However, should things be changed and this script ends up running on a system that doesn't automatically close the database connection, that would be a problem (that's not likely here but still).

 

To me, consistency is one of the key aspects of programming. And programmers should be making conscious decisions.

 

If it matters, than I guess it should be used for each branch following the specific logic of that branch. Following your example why not:

 

if($check){
// ok
mysqli_close($dbc); // perform cleaning like free statement, close query and close db, here just closing the db
header("Location: $url");
exit();
}
else {
// not ok, throw error
}
mysqli_close($dbc);

 

Yep, that's what it should be.

Link to comment
Share on other sites

 Share

×
×
  • Create New...