Nested loop used to insert an item into an array of objects

Question

I have 2 arrays. One is an array of objects $placementObjects, the other an associative array $ads which contains something like:

[
    'mobile' => [
        'inner' => [
            'units' => [
                3 => [
                    'pos' => 0,
                    'tag' => 'mobileAd:articles',
                ],
            ],
            'enabled' => '1',
        ],
        'sidebar' => [
            'units' => [
                4 => [
                    'pos' => -1,
                    'tag' => 'mobileSidebarAd:articles',
                ],
            ],
        ],
    ],
    'desktop' => [
        'inner' => [
            'units' => [
                1 => [
                    'pos' => 0,
                    'tag' => 'desktopAd:articles',
                ],
                2 => [
                    'pos' => 5,
                    'tag' => 'desktopAd:articles',
                ],
                3 => [
                    'pos' => 10,
                    'tag' => 'desktopAd:articles',
                ],
            ],
            'enabled' => '1',
        ],
        'sidebar' => [
            'units' => [
                4 => [
                    'pos' => -1,
                    'tag' => 'desktopSidebarAd:articles',
                ],
            ],
        ],
    ],
]

I need to insert each unit tag into the array of objects as specified by the pos (position), which are enabled. The array items without enabled key and/or pos value of -1 should be ignored. I created a function which contains the following snippet:

if ($paginate && count($placementObjects) === 1) {
    $totalObjects = count($placementObjects[0]);
} elseif ($paginate && count($placementObjects) > 1) {
    $totalObjects = $objectsPerPage;
} else {
    $totalObjects = count($placementObjects);
}

$totalPageObjects = ($objectsPerPage > $totalObjects) ? $totalObjects : $objectsPerPage;

if (!empty($placementObjects) && !empty($ads)) {
    foreach ($ads as $placement) {
        foreach ($placement as $item) {
            if (isset($item['enabled'])) {
                foreach ($item['units'] as $adUnit) {
                    if (-1 !== $adUnit['pos']) {
                        if ($paginate) {
                            foreach ($placementObjects as $key => $pageObjects) {
                                $countObjects = count($pageObjects);

                                if (0 === $adUnit['pos']) {
                                    array_splice($placementObjects[$key], $adUnit['pos'], 0, "[[{$adUnit['tag']}]]");
                                    continue;
                                } elseif ($totalPageObjects === $adUnit['pos'] && $countObjects >= $adUnit['pos']) {
                                    $pos = $adUnit['pos'] + (count($pageObjects) - $totalPageObjects);
                                    array_splice($placementObjects[$key], $pos, 0, "[[{$adUnit['tag']}]]");
                                    continue;
                                }

                                $pageCtr = 0;
                                foreach ($pageObjects as $index => $object) {
                                    if (is_object($object)) {
                                        if ($pageCtr === $adUnit['pos']) {
                                            array_splice($placementObjects[$key], $index, 0, "[[{$adUnit['tag']}]]");
                                            break;
                                        }

                                        $pageCtr++;
                                    }
                                }
                            }
                        } else {
                            if (0 === $adUnit['pos']) {
                                array_splice($placementObjects, $adUnit['pos'], 0, "[[{$adUnit['tag']}]]");
                                continue;
                            } elseif ($totalObjects === $adUnit['pos']) {
                                $pos = $adUnit['pos'] + (count($placementObjects) - $totalObjects);
                                array_splice($placementObjects, $pos, 0, "[[{$adUnit['tag']}]]");
                                continue;
                            }

                            $ctr = 0;
                            foreach ($placementObjects as $index => $object) {
                                if (is_object($object)) {
                                    if ($ctr === $adUnit['pos']) {
                                        array_splice($placementObjects, $index, 0, "[[{$adUnit['tag']}]]");
                                        break;
                                    }

                                    $ctr++;
                                }
                            }
                        }
                    }
                }
            }
        }
    }
}

Where:

$paginate is a boolean in which the value is passed as a parameter to the function.
$objectsPerPage is an int to specify how many items will be included in each page. Its value is also passed as a parameter to the function.

The outcome would be something like:

[
    0 => '[[desktopAd:articles]]',
    1 => object,
    2 => '[[mobileAd:articles]]',
    3 => object,
    4 => object,
    5 => object,
    6 => object,
    7 => object,
    8 => '[[desktopAd:articles]]',
    9 => object,
    10 => object,
    11 => object,
    12 => object,
    13 => object,
    14 => object,
    15 => '[[desktopAd:articles]]',
]

As it shows, I am using nested loops. While the function works as expected, I am afraid that the nested loops will cost me. What is the best approach to do this? How can I refactor my code to make it more efficient?

you're code is complex and scary looking but it's only handling 16 items and there's no recursion. i don't think you have anything to worry about as far as it costing you anything. even if there were 100 ad spots on your page that's still a pretty small number. before you try to improve performance you should improve the code complexity. break this code up into several small functions or even a class and then see what you can do to possibly improve performance. — I wrestled a bear once.
– I wrestled a bear once., Commented Nov 10, 2017 at 4:24

Conor Mancone · Accepted Answer · 2017-11-10 10:49:09Z

The fact that you are using nested loops shouldn't cause too much trouble. The reason is that, while your loops are nested, there is still only a fixed number of loops: your inner loop is still just going to loop once per "unit" in your overall data array. So all-in-all, I wouldn't expect the nature of this calculation to be inherently slow, unless of course you have hundreds of thousands of units to loop through. Then though, the problem won't be cause so much by the nested loops but rather by the fact that you have a lot of work to do.

The problem you do have is code readability. It is very difficult to tell what this is actually doing. That is partly because of the "complexity" of the task at hand (it's not actually very complex, but it has a number of steps that make it harder to follow), and your general code layout. The latter is fixable. I'm going to give just a couple suggestions though, because I have a general suggestion that may make all of this moot.

The first suggestion will help a lot and is also easy: always use guards! Instead of entering a new code block for each if condition, use a continue when you don't have to continue. Like so:

if (!empty($placementObjects) && !empty($ads)) {
    return [];
}

foreach ($ads as $placement) {
    foreach ($placement as $item) {
        if (empty($item['enabled'])) {
            continue;
        }

        foreach ($item['units'] as $adUnit) {
            if (-1 == $adUnit['pos']) {
                continue;
            }

            // code goes here
        }
    }
}

This will substantially improve the readability of your code. Losing a few levels of indentation will make everyone's life easier.

Next up, your inner loop is has this big if-else conditional on whether or not you are paginating. From what I can tell though, your code actually does the same thing in both cases. The only thing that changes is where it puts it. Finding a way to do the calculations just once and then having an if-else conditional on pagination decide where to put it in your results array will cut the duplication in half.

Another approach

However, I think your best bet is to not do this at all. You are effectively trying to clean up a giant mess (this large array is formatted for a different problem set than the one you are trying to solve), which results in a similarly-messy bit of code. The best answer, therefore, is to just not make a mess in the first place.

In this particular case what that means is to fix this problem earlier in the chain. Your data isn't just coming out of the database like this. I'm sure what is happening is that you are calling a function somewhere that is returning this array of data. That function is making some database calls and returning this structured data array. I'm sure it does that because someone somewhere needs this structured data array. In otherwords, this data array is built to solve a particular problem, but it isn't designed for your problem. Hence why you have to jump through loops to re-format it for your needs.

Rather than trying to modify this array to match your needs, I think you'll be better off going back to the database and reconstructing the data you need from there. Or see if you can modify the function that loads this gigantic array so that it can also load the data in the format you need at the same time. Those two answers can have their own downsides but I think it is definitely worth it to consider some completely different solutions.

Ultimately, the problem you are having is that you are trying to make a round peg fit into a square hole: this data array is not in a format that is a good fit to the problem you are trying to solve, so you are trying to adjust it to work better. However, the best answer is not to modify what you have but rather to start over with something that does work for you.

Rene Saarsoo · Accepted Answer · 2017-12-10 14:56:02Z

Use smaller functions

You say this code lives inside a function. But for some reason you don't give us the whole function, instead you're describing in text what the parameters and return value are.

Is it because your actual function contains lots of code unrelated to your question? Even when that's not the case, this function is way too large. You should break it up to smaller functions.

Breaking it up should also help you to eliminate duplication. Most notably the two branches of if ($paginate) contain mostly the same code.
Breaking it up will also help you to reduce nesting, which hinders readability.

Stack Exchange Network

Nested loop used to insert an item into an array of objects

2 Answers 2

Use smaller functions

You must log in to answer this question.

Hot Network Questions

Nested loop used to insert an item into an array of objects

2 Answers 2

Use smaller functions

You must log in to answer this question.

Related

Hot Network Questions