Member Avatar for ___150

Is this code correct? Or should I keep using } else { for (errors) until the end?

if ($y = $link->prepare('SELECT id, password FROM usuarios WHERE username = ?')){
$y->bind_param('s', $_POST['username']); $y->execute(); $y->store_result();
if ($y->num_rows > 0){ echo 'Username exists, please choose another!';
} else {

if ($y = $link->prepare('SELECT id, password FROM usuarios WHERE email = ?')){
$y->bind_param('s', $_POST['email']); $y->execute(); $y->store_result();
if ($y->num_rows > 0){ echo 'Email exists, please choose another!'; exit;
}

if (strlen($_POST['password']) > 25 || strlen($_POST['password']) < 6){
echo 'Password must be between 6 and 25 characters long!'; exit;
}

Another question, is it possible to find the code below and displaying different error messages?

Recommended Answers

All 12 Replies

I would do

if (...)
{

}
else if (...)
{

}
else if (...)
{

}
else
{

}

It's not good practice to short circuit the page by calling exit; in the middle of an if-else block.

You might also wish to look into try/catch blocks for errors.

Member Avatar for ___150

What would the code look like if I wanted to check multiple in a single query?

if ($y = $link->prepare('SELECT id, password FROM usuarios WHERE username = ?'))
{
    $y->bind_param('s', $_POST['username']);
    $y->execute();
    $y->store_result();

    if ($y->num_rows > 0)
    {
        show_error('Username exists, please choose another!');
    }
    else if ($y = $link->prepare('SELECT id, password FROM usuarios WHERE email = ?'))
    {
        $y->bind_param('s', $_POST['email']);
        $y->execute();
        $y->store_result();

        if ($y->num_rows > 0)
        {
            show_error('Email exists, please choose another!');
        }
        else if (strlen($_POST['password']) > 25 || strlen($_POST['password']) < 6)
        {
            show_error('Password must be between 6 and 25 characters long!');
        }
    }
}

function show_error($error_msg)
{
    echo $error_msg;
    exit;
}

It's not good practice to short circuit the page by calling exit; in the middle of an if-else block.

So you call a separate method that calls exit for you? All you have done is make the code more obscure by hiding the exit.
try/catch would be much clearer.

If exit does what I think it does then aren't you still short-circuiting by wrapping it in another function?

commented: Snap! +15

I was presenting two possible solutions. I converted their multiple if statements to if-else and I also showed you can put exit at the end of an error message function call. In PHP, exit exits the code gracefully, calling any shutdown functions that have been registered, etc.

I am perhaps biased because that is what Codeigniter, the MVC framework DaniWeb uses, chooses to do. They have a built-in function called show_error() that generates a friendly error page and then pushes it to the browser with a call to exit. Other built-in functions process views and then push those to the browser with a call to exit.

The point I was trying to make was that I am not a fan of if-else if-else blocks where some of the blocks randomly short circuit while others do not, and it becomes difficult to read the code later and follow what might happen. More importantly, it is easy to miss a short circuit placed in the middle of a block of code. The way it stands now, the only thing that happens as a result of executing all that code is short circuiting on all sorts of errors. The exit is refactored out so that it’s easier to see what’s going on. I’m not sure if I’m making sense.

I’m a massive believer in the importance of good naming.
If I see a call to show_error then I’d expect that to show the error and return, not shut down my whole program.
If your function were called exit_with_error_message then most of the code smell goes away. You’re still exiting from the middle of an if/else, but at least it’s obvious.

The point I was trying to make is that there is functionally little difference between

if condition
    exit
else if condition
    exit
else if condition
    exit
else
    something

more code

and

if condition
    show_error()
else if condition
    show_error()
else if condition
    show_error()
else
    something

more code

They all short circuit the structured code. But at least in the first case the short-circuiting is explicit. In the second case a casual reader would assume more code will always be executed. The second case is extremely misleading and should never be used.

If I see a call to show_error then I’d expect that to show the error and return, not shut down my whole program.

That makes very little sense in the context of web development. If an error is encountered, such that an error needs to be conveyed to the end-user, the typical behavior is that the user is presented with an HTML page that explains an error message and, most importantly, spits out an HTTP status code that corresponds to the specific error (404, etc.). That must end the script execution because, with the exception of HTTP 103 early hints, there can only be one HTTP status code per URI request.

The reason that CodeIgniter explicitly calls exit at the end of its show_error() function is because, if a bad programmer accidentally allowed the code to continue, and run to completion, it would most likely spit out an HTTP 200, which would break the script and produce undesired behavior.

And another thing…
If the if block exits, then the “else” following it is redundant.

commented: True +34

The point I was trying to make is that there is functionally little difference between

I disagree. While it's not apparent in the simple pseudocode that has been passed around this thread, in the real world, show_error() would parse a view, send a friendly HTML-based error page to the end-user's browser, and send a relevant HTTP status code to denote what specifically happened (e.g. 4xx errors for user-error, 5xx errors for server-error). It most likely will also send additional HTTP headers, such as no-cache headers, since an error has occurred. All before then using exit to begin executing the registered shutdown scripts. Placed into every else/else-if block, that's a lot of redundant code for every time we encounter a failing error that we want to display to the end-user.

I disagree (and you knew I would). If you had called the function show_error_and_exit() then perhaps.

Be a part of the DaniWeb community

We're a friendly, industry-focused community of developers, IT pros, digital marketers, and technology enthusiasts meeting, networking, learning, and sharing knowledge.