Form validation for user account creation

Question

What would be a cleaner way to write the following?

// Check if add user was attempted
if ($params['submit']) {
    // Verify all fields are filled in
    if (Utilities::checkAllFieldsNotEmpty($params)) {
        // Make sure username is not taken
        if (Admin::checkUsername($params['username'])) {
            // Check if directory is created
            if (Admin::createUsersDirectory($params['username'])) {
                if (Admin::createNewUser($params['name'], $params['username'], $params['password'], $params['admin'])) {
                    Utilities::setMessage("Excellent!", "User was created successfully.", "admin_modal");
                }
            } else {
                Utilities::setMessage("Whoa!", "Directory creation failed.", "admin_modal");
            }
        } else {
            Utilities::setMessage("Whoa!", "Username in use.", "admin_modal");
        }
    } else {
        Utilities::setMessage("Whoa!", "Please fill in all fields", "admin_modal");
    }

    // Return admin view with message
    return $this->view->render($response, 'admin/admin.twig', [
        'name' => $user['name'],
        'message' => $_SESSION['message'],
        'form' => [
            'name' => $params['name'],
            'username' => $params['username'],
            'admin' => $params['admin']
        ]
    ]);
}

Answer 1

Don't nest your conditions like this. Validate the input up front and fail out if need be.

For example that might look like this:

if(empty($params['submit'])) {
   // no parameters set
   // log error, throw exception, etc. as approrpiate
}
if(Utilities::checkAllFieldsNotEmpty($params) === false) {
   // fail out
   // not sure if you really need this condition if you validate each field
   // individually
}
// etc.

Then I would suggest that your Admin class should abstract away the details of user creation away from this script. Ideally this script could just have something like:

try {
    $user = Admin::createNewUser(...);
} catch (Exception $e) {
    // do something to handle exception
}

And createNewUser() would go through all the logic of for validating user name, setting up directories, etc. This code should know nothing about what is required to create a "user" outside of whatever information needs to be passed (i.e. user name).

Answer 2

Since at most one of the messages will be selected, you should invert the conditions to make it linear rather than nested. This has the benefit of putting each error message right next to its corresponding test.

if (!Utilities::checkAllFieldsNotEmpty($params)) {
    Utilities::setMessage("Whoa!", "Please fill in all fields", "admin_modal");
} elseif (!Admin::checkUsername($params['username'])) {
    Utilities::setMessage("Whoa!", "Username in use.", "admin_modal");
} elseif (!Admin::createUsersDirectory($params['username'])) {
    Utilities::setMessage("Whoa!", "Directory creation failed.", "admin_modal");
} elseif (Admin::createNewUser($params['name'], $params['username'], $params['password'], $params['admin'])) {
    Utilities::setMessage("Excellent!", "User was created successfully.", "admin_modal");
}

There are still a couple of problems, though.

First, I would say that the methods named check… do not clearly convey what they do. Based on those names, I would expect that they throw an exception if they fail the test. If they are actually predicates, it would be better to name them Utilities::areAllFieldsNotEmpty(…) and Admin::isUsernameAvailable(…).

A hint that something is wrong is that the Admin::createNewUser(…) call is not negated like the others. The problem is that you've forgotten to display an error message for that potential failure:

    …
} elseif (!Admin::createNewUser($params['name'], $params['username'], $params['password'], $params['admin'])) {
    Utilities::setMessage("Whoa!", "User creation failed!", "admin_modal");
} else {
    Utilities::setMessage("Excellent!", "User was created successfully.", "admin_modal");
}