Can this PHP code be simplified to improve performance?

Question

The goal of this code, is to get all brands for all stores into one array, and output this to the screen. If a brand exists in multiple stores, it will only be added once.

But I feel I have too many for loops, and that it might choke the CPU on heavy traffic. Is there a better solution to this?

  function getBrands($stores, $bl)
  {
    $html = "";

    //Loop through all the stores and get the brands
    foreach ($stores as $store)
    {
      //Get all associated brands for store
      $result = $bl->getBrandsByStore($store['id']);

      //Add all brands to array $brands[]
      while ($row = mysql_fetch_array($result))
      {
        //If this is the first run, we do not need to check if it already exists in array
        if(sizeof($brands) == 0)
        {
          $brands[] = array("id" => $row['id'], "name" => $row['name']);
        }
        else
        {
          // Check tosee if brand has already been added.
          if(!isValueInArray($brands, $row['id']))
            $brands[] = array("id" => $row['id'], "name" => $row['name']);
        }
      }
    }

    //Create the HTML output
    foreach($brands as $brand)
    {
      $url = get_bloginfo('url').'/search?brandID='.$brand['id'].'&brand='.urlSanitize($brand['name']);
      $html.= '<a href="'.$url.'" id="'.$brand['id'].'" target="_self">'.$brand['name'].'</a>, ';
    }

    return $html;
  }

  //Check to see if an ID already exists in the array
  function isValueInArray($values, $val2)
  {
    foreach($values as $val1)
    {
      if($val1['id'] == $val2)
        return true;
    }
    return false;
  }

Answer 1

How are your tables designed? If you had a store table, a brand table, and a link table that had the relationship between stores and brands, you could just pull in the list of brands from the brand table in one query and not have to do any other logic.

Design your tables so they easily answer the questions you need to ask.

Answer 2

If you need to get all the brands for a certain set of stores then you should consider using a query crafted to do that instead of iterating through all the stores and getting the separate pieces of information.

Answer 3

No need to loop through two sets of arrays (one to build up the array of brands, and then one to make the HTML). Especially since your helper function does a loop through -- use the array_key_exists function and use the ID as a key. Plus you can use the implode function to join the links with ', ' so you don't have to do it manually (in your existing code you'd have a comma on the end you'd have to trim off). You can do this without two sets of for loops:

function getBrands($stores, $bl) 
{
    $brands = array();

    //Loop through all the stores and get the brands
    foreach ($stores as $store)
    {
        //Get all associated brands for store
        $result = $bl->getBrandsByStore($store['id']);

        //Add all brands to array $brands[]
        while ($row = mysql_fetch_array($result))
        {
            if (!array_key_exists($row['id'])
            {

                $url = get_bloginfo('url') . '/searchbrandID=' . 
                       $brand['id'] . '&brand=' . urlSanitize($brand['name']);
                $brands[$row['id']] .= '<a href="' . $url . '" id="' . 
                                       $brand['id'] . '" target="_self">' . 
                                       $brand['name'] . '</a>';
            }
        }
    }

    return implode(', ', $html);
}

That will get you the same effect a little faster. It's going to be faster because you used to loop through to get the brands, and then loop through and build up the HTML. Don't need to do that as two separate loops so it all at once and just store the HTML as you go along. Plus since it's switched to use array_key_exists, instead of the helper you wrote that checks by looping through yet again to see if a brand is in there, you'll see more speed improvements. Hashmaps are nice like that because each element in the hashmap has a key and there are native functions to see if a key exists.

You could further optimize things by writing a better SQL statement with a distinct filter to make it so you don't have to do a while inside a foreach.

Answer 4

From your comment, you mention "Guide table has X stores and each store has Y brands". Presumably there's a "stores" table, a "brands" table, and a "linkage" table, that pairs store_id to brand_id, in a one-store-to-many-brands relationship, right?

If so, a single SQL query could do your task:

SELECT b.`id`, b.`name`
FROM `stores` s
LEFT JOIN `linkage` l
  ON l.`store`=s.`id`
LEFT JOIN `brands` b 
  ON b.`id`=l.`brand`
GROUP BY b.`id`;

That final GROUP BY clause will only show each brand once. If you remove it, you could add in the store ID and output the full list of store-to-brand associations.