1
\$\begingroup\$

I am new to PHP and was just wondering if there is a better way to write the following OR if it would make any difference to use elseif here instead of multiple if statements - especially if the options in questions might be more than in the example here (the current code is working).

Note: I cannot use switch here since the haystack string can contain multiple of the needle strings.

$errorList .= "Some text <ul>";
    if(strpos($errorDetails, "emailInvalid") !== false){
        $errorList .= "<li>Some text</li>";
    }
    if(strpos($errorDetails, "passLength") !== false){
        $errorList .= "<li>Some text</li>";
    }
    if(strpos($errorDetails, "passLowerCase") !== false){
        $errorList .= "<li>Some text</li>";
    }
    if(strpos($errorDetails, "passUpperCase") !== false){
        $errorList .= "<li>Some text</li>";
    }
    if(strpos($errorDetails, "passNumberSymbol") !== false){
        $errorList .= "<li>Some text</li>";
    }
    if(strpos($errorDetails, "passMatch") !== false){
        $errorList .= "<li>Some text</li>";
    }
$errorList .= "</ul>";
echo $errorList;
\$\endgroup\$
8
  • 1
    \$\begingroup\$ Would a switch without breaks be acceptable? \$\endgroup\$
    – Mast
    Commented Jul 4, 2015 at 10:45
  • 1
    \$\begingroup\$ I am downvoting this question because <li>Some text</li> is not, or will not be, your real code. As noted in the help center, Code Review prefers real code. \$\endgroup\$ Commented Jul 4, 2015 at 11:17
  • 2
    \$\begingroup\$ @Mast I don't think a switch without breaks would work. \$\endgroup\$ Commented Jul 4, 2015 at 11:18
  • 1
    \$\begingroup\$ @SimonAndréForsberg: A downvote because I am not posting real text from my website ?? \$\endgroup\$
    – keewee279
    Commented Jul 4, 2015 at 11:37
  • 1
    \$\begingroup\$ @keewee279 Honestly, yes. It was either that or a close vote, but I felt that it was still possible to review it. See this meta question. If you want to discuss more about this and want me to explain my downvote further, you are welcome to chat in The 2nd Monitor \$\endgroup\$ Commented Jul 4, 2015 at 11:43

2 Answers 2

9
\$\begingroup\$

I would use an associative array (also known as a "map" or "dictionary" in other languages).

$errorMessages = array('emailInvalid' => 'Invalid email',
    'passLength' => 'invalid password length',
    'passLowerCase' => 'missing lower case character in password'
    // and so on...
); 

Then I would loop across this map:

foreach ($errorDetails as $key => $value) {
    if (strpos($errorDetails, $key) !== false) {
        $errorList .= '<li>' . $value . '</li>';
    }
}

strpos

Storing your error-message stubs in $errorDetails is inefficient. strpos loops through the string each time. I'd recommend to instead append your error messages to an array, which would open up the possibility of looking up the full error message from something like emailInvalid with a O(1) complexity.

How did I know it was a map that was needed?

Let's look at this code:

if(strpos($errorDetails, "emailInvalid") !== false){
    $errorList .= "<li>Some text</li>";
}
if(strpos($errorDetails, "passLength") !== false){
    $errorList .= "<li>Some (other) text</li>";
}

There are two things that differs here, these parts can be simplified to:

if (strpos($errorDetails, $xxxxxxx) !== false) {
    $errorList .= $yyyyyyy;
}

To clean up this code, we could extract a method and call that multiple times, such as:

addErrorList($errorDetails, 'emailInvalid', 'invalid e-mail');
addErrorList($errorDetails, 'passLength', 'invalid password length');
...

However, this is still not very nice. A map / associative array contains a key to value mapping, which is precisely what you have here. For each specific error detail key, there is an associated error message. So add those values to an associative array and then loop through it.

\$\endgroup\$
6
\$\begingroup\$

To add just a little bit to @Simon's excellent answer:

I [...] was just wondering [...] if it would make any difference to use elseif here instead of multiple if statements

Uhm, yes, it would probably not do what you want: after any of the if statements are true, all the elseif not be evaluated, so only one error type would be printed at a time.

Another small thing, instead of accumulating all the error messages in $errorList, you could echo them directly.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for this ! If I would echo them directly how would I manage that they appear as an unordered list ? \$\endgroup\$
    – keewee279
    Commented Jul 5, 2015 at 2:05

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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