How to improve this nested PHP if/else structure?

Question

I've written some code, it might not even run, not tried yet. But I've written it all with if else pairs where needed. I can't think of a better way of writing it. Is there a better way that's obvious to you?

The code is going to go into the head of the script and intercept the building of the page if a form with the field "email_notification" has been submitted. If so, it does it's thing, checking if they've entered an email address, and if the item they're subscribing to exists, then adds them, giving error status messages with each else statement. Eventually, it'll redirect to another page with the result of what's happened and kill execution of the rest of the script.

Thanks.

if (isset($_POST['email_notification'])) {
if (!empty($_POST['software_id'])) {
    if (!empty($_POST['email_address'])) {
        if ($result = mysql_query("SELECT shortname, fullname FROM soft_data WHERE shortname = '{$_POST['software_id']}'")) {
            $fullname = mysql_fetch_array($result);
            if (false !== mysql_query("INSERT INTO email_subs(soft_name, email_address) VALUES('{$_POST['software_id']}', '{$_POST['email_address']}')")) {
                $status = "Your email address will be notified when " . $fullname['fullname'];
            }
        } else {
            $status = "Software ID not found.";
        }
    } else {
        $status = "No email address entered.";
    }
} else {
    $status = "No software ID entered.";
}
header("Location: http://example.com/subscription/?status=$status");
die();
}

I hope you've read up on http://stackoverflow.com/questions/332365/xkcd-sql-injection-please-explain — Marvin Smit, Feb 14 '11 at 14:14
Oh,... I should have explained, I'm well aware how raw and unsecured the code is. It's only just been written. It'll all be very safe and secure from SQL injection when it's done. I just want help with improving the logic of this nesting first. Thanks. — i-CONICA, Feb 14 '11 at 14:18

score 1 · Answer 1 · answered Feb 14 '11 at 14:17

For example :

$errors = array();
if (empty($_POST['software_id'])) $errors[] = 'No software ID entered.';
if (empty($_POST['email_address'])) $errors[] = 'No email address entered.';
...
if (count($errors)) {
  // display errors
} else {
  // process
}

score 0 · Accepted Answer · answered Feb 14 '11 at 14:18

I'd either embed the lot in a method and return the status (string) or true (if successful), or build a helper-method that outputs the status and ends the script like following:

<?php
function reportStatus($status) {
    header("Location: http://example.com/subscription/?status=$status");
    exit();
}

if (!isset($_POST['email_notification']))
    reportStatus("No email address entered.");
if (empty($_POST['software_id']))
    reportStatus("No software ID entered.";
if (empty($_POST['email_address']))
    reportStatus("No email address entered.");
if (false === $result = mysql_query("SELECT shortname, fullname FROM soft_data WHERE shortname = '{$_POST['software_id']}'"))
    reportStatus("Software ID not found.");
$fullname = mysql_fetch_array($result);
if (false !== mysql_query("INSERT INTO email_subs(soft_name, email_address) VALUES('{$_POST['software_id']}', '{$_POST['email_address']}')"))
    reportStatus("Your email address will be notified when " . $fullname['fullname']);
    //Is above actual an error??
/* Rest of script processing goes here*/
?>

Hi, Rudu. Thanks for the comment. The last part in your code isn't an error, that's the successful end of it all, if it doesn't fail before hand. I much prefer the logic of yours and the above poster's methods. I'll adopt one of, or a little of both of these. Thanks. — i-CONICA, Feb 14 '11 at 14:39

score 0 · Answer 3 · answered Feb 16 '11 at 14:24

You can also do something like this for quick variable presence check:

function reportStatus($status) {
    header("Location: http://example.com/subscription/?status=$status");
    exit();
}

$ar = array(
    'email_notification' => null,
    'software_id'        => 'No software ID entered.',
    'email_address'      => 'No email address entered.'
);

$df = array_diff($ar, $_POST);

if (!empty($df)) {
    $status = current($df);
    if (!empty($status)) {
        report_status($status);
    }
}

And than continue with your code...

How to improve this nested PHP if/else structure?

3 Answers3