Switch case within foreach loop duplicating results

The below query is producing duplicate results for me:

foreach($roles as $role){ 
      if (($role == "Lead") or ($role == "developer") or ($role == "cms"))
          {
           <img src="https://asia.olympus-imaging.com/product/dslr/em1mk3/sample.html" />
          }
     else 
          {
           <p>No image</p>
          }
        }

in this query, for people who has multiple roles like - Lead and CMS, see duplicate output of t he images. What could be the better way to handle this?

CodePudding user response：

Just write a utility function:

function shouldShowImage($roles)
{
    foreach($roles as $role)
    {
        if(($role == "Lead") or ($role == "developer") or ($role == "cms"))
            return true;
    }
    return false;
}

if(shouldShowImage($roles))
    echo '<img src="...">';
else
    echo '<p>No image</p>';

CodePudding user response：

The problem of this code is that the logic is broken. You have multipe cases (Lead, developer, cms) but only want to match one of them. The same problem occurs when not matching logic is supposed to be run. The first idea is to leave the loop as soon as a role matches. This is what Kevin Y said. And it's the same what the utility function does (because of the return inside the loop). But it's not the best idea. Let's take a look:



    // Not good! Just for explanation
    foreach ($roles as $role) { 
        if (($role == "Lead") or ($role == "developer") or ($role == "cms")) {
            echo "whatever";
            break;
        } else {
            echo "somewhatelse";
            // we should break here but can't for logical reasons
        }
    }

In this example, we leave the loop when a role match is found. But what if the role does not match? May be it will not match multiple times. So you get multiple "no-matching-image". This is not what you want. Unfortunately, we can't leave the loop in case of no match because we have to test more cases...

So, when you think again what you want, you come to this result: If one of multiple cases match, show a particular image (and stop testing other cases). If not, show a different image. Hehe, this sounds like using an array!



    // Should be better
    $match_roles = ["Lead", "developer", "cms"];
    $match = false;
    
    foreach ($roles as $role) {
        if (in_array($role, $match_roles)) {
            $match = true;
            break;
        }
    }
    
    echo (true === $match) ? "one_image" : "another_image";

By the way: You can extend matching cases in this code very easily, just by adding more elements to the $match_roles array.

Hope this helps.