3
\$\begingroup\$

I am writing a function which loops through an array of products. The function needs to return:

  • the ID of the first product that has an image and is in stock
  • if no products have an image and are in stock, then return the first product id that has an image
  • if no products have an image, then return the first product id that is in stock
  • if no products have an image, and no products are in stock, return the first product id

This is what I have written, and it is working as expected:

function loopThroughProducts($products)
{
    $firstId = $products[0]->id;

    foreach ($products as $product) {
        if ($product->hasImage && $product->inStock) {
            $id = $product->id;
            return $id;
        }
        if ($product->hasImage && empty($firstIdWithImage)) {
            $firstIdWithImage = $product->id;
        }
        if ($product->inStock && empty($firstIdInStock)) {
            $firstIdInStock = $product->id;
        }
    }

    if (!empty($firstIdWithImage)) {
        return $firstIdWithImage;
    }
    if (!empty($firstIdInStock)) {
        return $firstIdInStock;
    }

    return $firstId;
}

Does anyone see a way that I can optimize this loop or simplify the logic? I dislike using so many sequential if-statements in one function, and there are some minor violations to the DRY principle.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ I have always heard if you are indenting more than twice in a function or have lots of if..else you should separate this functionality. If this is part of a class you could break all of these into methods that are more flexible and testable. \$\endgroup\$ – Ross Edman Oct 20 '14 at 17:26
  • \$\begingroup\$ Thanks for the feedback. I believe you are referring to cyclomatic complexity. For our project, if-statements nested within a loop are generally acceptable. Any additional nesting would certainly provoke ancillary private methods as you described. \$\endgroup\$ – Leo Galleguillos Nov 8 '14 at 19:34
1
\$\begingroup\$

Clean, readable code. But why not use more of PHP's standard array functions? Use the array_search or in_array functions. Or -even more fancy- the array_filter function.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for the feedback. Those are definitely viable options if I was matching only one set of criteria. However, since I am matching on a few different criteria (as described in the bullet points of my question), my function conveniently returns all the matches I need in one loop. If I used array_search or in_array, I would be searching (looping through) the same array at least three times, and this would not be as performant. \$\endgroup\$ – Leo Galleguillos Oct 19 '14 at 21:21
1
\$\begingroup\$
  • Yes, you can certainly nest your checks a bit better to improve DRYness.
  • isset() has an advantage over empty() in this scenario because isset() accepts multiple parameters versus empty() only permits a single parameter.
  • using a chain of null coalescing operators will reduce the lines of code near the end of your custom function. Here is a reference to this technique.

For anyone who got lost in your explanation of the logic:

  1. Has Image, Has Stock -- use as soon as found
  2. Has Image, No Stock -- use if no #1 is found in whole array
  3. No Image, Has Stock -- use if no #2 is found in whole array
  4. No Image, No Stock -- use default if no #3 is found in whole array

Logically, there is no reason to store an id that qualifies as #3 if a #2 id is already found.

Code: (not tested because no test data provided)

function loopThroughProducts($products) {
    foreach ($products as $product) {
        if ($product->hasImage) {
            if ($product->inStock) {
                return $product->id;
            } elseif (!isset($hasImageNoStock)) {
                $hasImageNoStock = $product->id;
            }
        } elseif ($product->inStock && !isset($hasImageNoStock, $hasStockNoImage)) {
            $hasStockNoImage = $product->id;
        }
    }
    return $hasImageNoStock ?? $hasStockNoImage ?? $products[0]->id;
}
\$\endgroup\$

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.