4
\$\begingroup\$

I have a script that checks if the specified set of $_POST variables are supplied and not empty and returns an error string that is human readable.

Here's my code:

if ($_SERVER["REQUEST_METHOD"] === "POST") {
    // Local variables initialization
    $missing = [];

    $checkVariables = [
        'name', 'address', 'contact'
    ];

    // Check for nulls or non existent payload
    foreach ($checkVariables as $item) {
        if (empty($_POST[$item]) || !array_key_exists($item, $_POST)) {
            $missing[] = $item;
        }
    }

    $missingCount = count($missing);
    if ($missingCount > 0) {
        $missingString = '';

        if ($missingCount === 1) {
            $missingString = $missing[0];
        } else if ($missingCount === 2) {
            $missingString = implode(' and ', $missing);
        } else {
            $lastItem = array_pop($missing);
            $partialMissingString = implode(', ', $missing);

            $missingString = $partialMissingString . ", and {$lastItem}";
        }

        $verb = $missingCount > 1 ? 'are' : 'is';
        $field = $missingCount > 1 ? 'fields' : 'field';
        $errorString = "Incomplete information received. " .
            "The {$missingString} {$field} {$verb} missing. " .
            "Please check your inputs and try again.";

        // Exits the script
        OutputResponse::sendResponse(400, $errorString);
    }
    // if there's no missing continue running the script
    // more code here but it's not the focus of this code review
}

My OutputResponse code:

class OutputResponse
{
    public static function sendResponse(string $statusCode, $messageBody): void
    {
        if ($statusCode == 500) {
            header("HTTP/1.1 500 Internal Server Error");
        } else if ($statusCode == 405) {
            header('HTTP/1.1 405 Method Not Allowed');
        } else if ($statusCode == 404) {
            header("HTTP/1.1 404 Not Found");
        } else if ($statusCode == 403) {
            header("HTTP/1.1 403 Forbidden");
        } else if ($statusCode == 401) {
            header("HTTP/1.1 401 Unauthorized");
        } else if ($statusCode == 400) {
            header("HTTP/1.1 400 Bad Request");
        } else {
            header("HTTP/1.1 200 OK");
        }

        header('Content-Type: application/json');
        echo json_encode([
            'code' => $statusCode,
            'data' => $messageBody
        ]);

        exit;
    }
}

The script expects $_POST['name'], $_POST['address'], and $_POST['contact'] to be not null or empty. The array keys to be checked are in the $checkVariables array.

For simplification, I'm dropping the $_POST in the next examples.

If name is the only one missing:

{
    "code": "400",
    "data": "Incomplete information received. The name field is missing. Please check your inputs and try again."
}

If name and contact are missing:

{
    "code": "400",
    "data": "Incomplete information received. The name and contact fields are missing. Please check your inputs and try again."
}

If all three are missing:

{
    "code": "400",
    "data": "Incomplete information received. The name, address, and contact fields are missing. Please check your inputs and try again."
}

Personally, the part on the if ($missingCount > 0) block is the messiest or unoptimized.

Any improvements or critiques are welcome!

\$\endgroup\$
1
  • \$\begingroup\$ I would wrap the field name in quotes/double quotes in your error message to indicate that it's being referred to as a word. e.g. The field "foo" is missing \$\endgroup\$ – Robert Rocha Feb 22 at 21:18
3
\$\begingroup\$
  1. if (empty($_POST[$item]) || !array_key_exists($item, $_POST)) { is literally checking if $_POST[$item] is not declared or is falsey THEN checking if $_POST[$item] is not declared.

    // Check for nulls or non existent payload
    

    The comment associated with the step indicates that you want to check if $_POST elements are not declared or are null. In this case, you should only use:
    if (!isset($_POST[$item])) {.

    If you want to check that elements are not declared or empty/falsey, then use:
    if (empty($_POST[$item])) {.

  2. if ($missingCount > 0) { can be safely reduced to if ($missingCount) { which checks if the count is "truthy" (in other words, not zero).

  3. else if is one word in php, please always type elseif to comply with PSR guidelines.

  4. Don't bother declaring $missingString = ''; if you are unconditionally going to overwrite it anyhow.

  5. You can combine the if and first else if because if the array only has one element, then no glue will be used while producing the output string.

    if ($missingCount < 3) {
         $missingString = implode(' and ', $missing);
    }
    
  6. The "smart-implode" as I've seen it referred to at least once on StackOverflow is fine enough. There are a few other ways to do it -- I'll show another one below.

  7. The whole chunk of code that crafts the OutputResponse might look like this: (Demo). The advantages are: fewest declared variables (specifically no single-use variables), not repeated function calls, and the elegance of sprintf() when injecting variables into a string.

    $missingCount = count($missing);
    if ($missingCount) {
        if ($missingCount < 3) {
             $glue = ' and ';
        } else {
             $glue = ', ';
             $missing[] = 'and ' . array_pop($missing);
        }
        OutputResponse::sendResponse(
            400,
            sprintf(
                'Incomplete information received. The %s %s missing. Please check your inputs and try again.',
                implode($glue, $missing),
                $missingCount === 1 ? 'field is' : 'fields are'
            )
        );
    }
    
  8. [EDITED] After reading the other reviews and learning that my recommended lookup array was incomplete, flawed, and frankly suboptimal, I now find myself agreeing with YourCommonSense's advice to use http_response_code() for setting the HTTP response code. See this Stack Overflow page for additional considerations and fringe cases around the native function which was available since PHP5.4. Also, Now I suppose my new code for the sendResponse() method would look like this:

     http_response_code($statusCode);
     header('Content-Type: application/json');
     exit(
         json_encode([
             'code' => $statusCode,
             'data' => $messageBody
         ])
     );
    
\$\endgroup\$
5
  • \$\begingroup\$ There's a minor bug with your lookup where you could output (for example) a 301 OK response header \$\endgroup\$ – Steven Feb 6 at 0:00
  • \$\begingroup\$ I intentionally did not change the behavior of the OP's code. I did not know if they specifically wanted this behavior. \$\endgroup\$ – mickmackusa Feb 6 at 0:03
  • \$\begingroup\$ Sorry, but you have (maybe unintentionally), changed the behaviour of the OPs code. In three ways: First the $statusCode returned is a formatted header and not the integer that the OP has, secondly the code above exits without sending any header so actually a 200 OK header will be sent not the 400 Bad Request which the OP intends, and third the formatted header (which isn't set and is instead returned to the wrong place) will always be XXX OK unless it's explicitly in the $codes array; the OP originally output a 200 OK if $statusCode didn't match \$\endgroup\$ – Steven Feb 6 at 0:27
  • \$\begingroup\$ Also, I'm aware that that probably sounds far more confrontational than I intended! Apologies for that, it's tricky to get the point across with limited characters without being direct \$\endgroup\$ – Steven Feb 6 at 0:28
  • 1
    \$\begingroup\$ Ah, I see what you mean. I'll edit when I get out of my garden. The function declaration and the header were omitted because I was only addressing the lookup (not providing a full function body. \$\endgroup\$ – mickmackusa Feb 6 at 0:30
3
\$\begingroup\$

OutputResponse

Quotation marks

This is hugely pedantic: your 405 header response uses single quotes where as all of the others use double quotes. It makes no difference (in fact single quotes are slightly faster for plain text in PHP), however, it's good practice to stick to one method especially when setting near identical strings.

Type hints & Parameters

You have a string type hint on your integer parameter and no type hint at all for your $messageBody parameter?

Currently you pass an integer...

OutputResponse::sendResponse(400, $errorString);

... to a parameter of type string...

function sendResponse(string $statusCode, $messageBody)

... and then compare it as an integer...

if ($statusCode == 500)
else if ($statusCode == 405)
...

... which is confusing on many levels and presumably not intended behaviour?

So that needs to be fixed, while there I would also swap your parameter order around too; then you can set a default value of, for example, 200 for your $statusCode.

public static function sendResponse(string $statusCode, $messageBody): void

// Becomes....

public static function sendResponse(string $messageBody, int $statusCode = 200) : void

HTTP Status: logic

Currently you're using a series of if/else statements to work out what HTTP response code to send. This can be tricky to read and isn't always clear what the code block attempts to do; switch is pretty much designed for this exact use case although personally I would use an array for this type of task.

Also, I wouldn't put the header(...) function call in every if statement. Much better to put one call at the end and return the string from the logic.

One more note is that you have your code set up to default to 200 OK if the response code doesn't exist in the block. This is wrong IMO: if your own code doesn't know what response to give then I doubt everything is OK? I'd opt for something like a generic 500, because the server has obviously run into a problem!

Note: 500 doesn't automatically mean fatal

Using switch:

$header = "HTTP/1.1 500 Internal Server Error";
switch $statusCode{
    case 500:
        $header = "HTTP/1.1 500 Internal Server Error";
        break;
    case 405:
        $header = "HTTP/1.1 405 Method Not Allowed";
        break;
    case 404:
        $header = "HTTP/1.1 404 Not Found";
        break;
    case 403:
        $header = "HTTP/1.1 403 Forbidden";
        break;
    case 401:
        $header = "HTTP/1.1 401 Unauthorized";
        break;
    case 400:
        $header = "HTTP/1.1 400 Bad Request";
        break;
} 

header($header);

Using an array:

$httpStatus = [
    500 => "HTTP/1.1 500 Internal Server Error",
    405 => "HTTP/1.1 405 Method Not Allowed",
    404 => "HTTP/1.1 404 Not Found",
    403 => "HTTP/1.1 403 Forbidden",
    401 => "HTTP/1.1 401 Unauthorized",
    400 => "HTTP/1.1 400 Bad Request",
    200 => "HTTP/1.1 200 OK",
];

header($httpStatus[$StatusCode] ?? $httpStatus[500]);

HTTP status: code

I'm not 100% onboard with sending a 400 Bad Request response in this scenario.

Perhaps you can call this personal preference or what not but as far as I can tell this is some form of ajax style set up? In which case the client side script is sending a request to the server which then acts upon it. Your server side script then gets hold of the header content and makes it's own checks... Which means that the HTTP request did succeed, the script (resource) was executed, and content was even generated.

The fact is, it isn't a HTTP issue, the user just hasn't submitted the correct details. You obviously implement your own error reporting to come back with application errors - so just set a code or a message in there (which is what you have done anyway) and leave the HTTP response code alone.

Main Code

REQUEST_METHOD

There's really no need for your opening (all encompassing) if statement. The whole purpose of the code block is to check that the $_POST variables are present and correct and if they aren't stop the execution of further code with exit;. By using the === comparison operator all you're doing is opening yourself up to issues in the event of a badly structured request where the method isn't formed as all caps (of course, this shouldn't be a problem in 2021 but you can't be sure).

Again, if the REQUEST_METHOD isn't POST then the rest of the code fails and exits anyway. So why waste the time checking?

if: logic

The second condition in your first if statement is redundant: if the index doesn't exist then the first condition is effectively empty(null) which, of course, is true. So checking afterwards to see if the array key exists isn't doing anything extra.

if (empty($_POST[$item]) || !array_key_exists($item, $_POST))

// Becomes...

if (empty($_POST[$item]))

Later on you use $missingCount > 0 which is okay, but... Not needed: count will return either 0 or some positive value greater than 0. Logically 0 == false so you can just skip the comparison; if the value is 0 then it's false, otherwise, its true.

if ($missingCount > 0)

// Becomes...

if ($missingCount)

String assembly

Custom error messages are tricky: they have to be informative but succinct, forceful but polite, etc. You don't want to annoy your user with a lengthy error message or, worse still, intimidate them with overly aggressive language.

So, my suggestion, would be to simply change the error message:

$errorString   = "Incomplete information received. Please check the following fields:";

Now all you need to do is append your variables to the end of the string. Which means we can reduce your code to a simple loop:

foreach ($checkVariables as $item) {
    $errorFields .= empty($_POST[$item]) ? "{$item}, " : "";
}

This leaves you with a trailing comma and no and so we do need to do a little formatting; but now you have a piece of code that scales automatically if you need to add more variables for example maybe you start collecting $_POST["phoneNumber"].

$errorString .= rtrim(preg_replace("/, (\w+), $/", " and $1", $errorFields), ", ");

This, I believe, is technically also the fastest method despite implementing regular expressions

If you're against changing your error message this is compatible with your current logic to work out plural etc. just use $errorFields and then concatenate as you see fit.

Updated Code

Main Body

$checkVariables = ['name', 'address', 'contact'];
$errorString    = "Incomplete information received. Please check the following fields: ";
$errorFields    = "";

foreach ($checkVariables as $item) {
    $errorFields .= empty($_POST[$item]) ? "{$item}, " : "";
}

$errorString .= rtrim(preg_replace("/, (\w+), $/", " and $1", $errorFields), ", ");

if ($errorFields) {
    OutputResponse::sendResponse(400, $errorString);
}

OutputResponse

class OutputResponse
{
    public static function sendResponse(string $messageBody, int $statusCode = 200) : void
    {
        $httpStatus = [
            500 => "HTTP/1.1 500 Internal Server Error",
            405 => "HTTP/1.1 405 Method Not Allowed",
            404 => "HTTP/1.1 404 Not Found",
            403 => "HTTP/1.1 403 Forbidden",
            401 => "HTTP/1.1 401 Unauthorized",
            400 => "HTTP/1.1 400 Bad Request",
            200 => "HTTP/1.1 200 OK",
        ];

        header($httpStatus[$StatusCode] ?? $httpStatus[500]);
        header('Content-Type: application/json');

        echo json_encode([
            'code' => $statusCode,
            'data' => $messageBody
        ]);

        exit;
    }
}

or, my preference...

class OutputResponse
{
    public static function sendResponse(string $messageBody, int $statusCode = 200) : void
    {
        header('Content-Type: application/json');

        echo json_encode([
            'code' => $statusCode,
            'data' => $messageBody
        ]);

        exit;
    }
}
\$\endgroup\$
17
  • \$\begingroup\$ I see some redundant review points with this review that already exist in other answers. Please spare the OP re-reading the same insights by only mentioning new insights. If you agree with / support insights from other reviewers, you can express your agreement in your answer without repeating it. \$\endgroup\$ – mickmackusa Feb 6 at 0:06
  • \$\begingroup\$ @mickmackusa I did check the guidelines before posting because I wasn't sure whether to remove them or not... But the only real duplicate is probably > 0 (and potentially empty()) everything else seems - to me at least - have a different slant? But who knows, I'm knew here and wasn't sure on etiquette (also I figured an answer has been accepted already so it didn't matter too much) \$\endgroup\$ – Steven Feb 6 at 0:34
  • \$\begingroup\$ At the end of the day, my stance on your posting style is that it errs on the side of generosity and therefore raises the expected standard of other reviews. For that reason, I hope that you settle in and continue reviewing. And yes, perhaps the truthy check is the only redundancy. \$\endgroup\$ – mickmackusa Feb 6 at 0:38
  • \$\begingroup\$ @mickmackusa a review is like a sculpture - written the way one sees it. I don't think there should be any restrictions in this regard. I would rather love to see http_response_code() used in your answers. Although I do understand that a lookup array is used as a generic example for fixing this kind of code but why stop here if you have even better optimization for this specific case? \$\endgroup\$ – Your Common Sense Feb 6 at 6:03
  • 1
    \$\begingroup\$ So this seems to have taken a turn I did not expect! I obviously did "read the other reviews" before posting, I thought, that was clear as I have also commented on both answers? This review only *really duplicates the two points I mentioned in an earlier comment(?) and even then the explanation is different. I would hope that explanation isn't considered mansplaining it just clarifies what is actually going on and why the additional code is redundant; is it not better to explain reasoning so that the OP doesn't blindly go off and use similar practices without understanding? \$\endgroup\$ – Steven Feb 6 at 10:56
2
\$\begingroup\$

An interesting approach.

Being a dev, I never bothered myself with proper English syntax, using just implode and calling it a day. but yours looks definitely better. Regarding optimization, you may notice that a condition $missingCount === 2 is actually covered by else case, so it can be removed. Or so I think because I believe that the last comma is superfluous in name, address, and contact.

Also, a trick I myself learned not long ago, is http_status_code() function which alone replaces the whole if ($statusCode == 500) { ... ladder, sending the proper header when called. So you can make it just

http_response_code($statusCode);

and call it a day.

It will also eliminate that awkward type juggling when you are sending a string status to the function and then have to compare it using == operator.

\$\endgroup\$
1
  • \$\begingroup\$ I would have guessed that the type juggling int >> string >> int isn't intentional and the type hinting should be updated? \$\endgroup\$ – Steven Feb 5 at 23:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.