Compare multidimensional arrays

Question

I'm writing a wordpress plugin that makes changes to the admin menu. To add some context: The wordpress global $menu is a multidimensional array. Each array in $menu represents a menu item, with the title, url, css classes etc at specific indexes in the array. the oa_custom_menu option is a similarly structured multidimensional array, but each menu item may have additional indexes in its array to store things like wether the menu item should be hidden or not.

the oa_custom_menu array is only updated when changes are made in my custom admin page. The $menu array may have changed since the last time this happened (e.g. new plugins or themes being activated and adding menu items), so I need to check for any added or removed items in the array.

index 2 in both menu arrays are the URL and the only thing that will consistently be unique. $custom_menu_item[7] is a bool that indicates wether the item should be hidden or not, so we ignore these items if they are hidden..

I hope that all makes sense, I'm not the best at explaining things!

The code I have works perfectly well, it just seems like too many foreach loops to me, but I can not for the life of me figure out a better way to do it. Am I right?

public function compare_menu_arrays() {

    global $menu;
    $custom_menu = get_option('oa_custom_menu') ? get_option('oa_custom_menu') : $menu;

    $menu_sort_keys = array();
    $custom_menu_sort_keys = array();

    foreach ( $menu as $menu_item ) {
        $menu_sort_keys[] = $menu_item[2];
    }

    foreach ( $custom_menu as $custom_menu_item ) {
        if ( empty($custom_menu_item[7]) ) {
            $custom_menu_sort_keys[] = $custom_menu_item[2];
        }
    }

    $new_menu_items     = array_diff( $menu_sort_keys, $custom_menu_sort_keys );
    $removed_menu_items = array_diff( $custom_menu_sort_keys, $menu_sort_keys );

    /*
     * add any new $menu items to our $custom_menu
     */
    if ( !empty($new_menu_items) ) {
        foreach ( $menu as $menu_item ) {
            if ( in_array( $menu_item[2], $new_menu_items ) ) {
                $custom_menu[] = $menu_item;
            }
        }
    }

    /*
     * remove any missing $menu items from our $custom_menu
     */
    if ( !empty($removed_menu_items) ) {
        foreach ( $custom_menu as $index => $custom_menu_item ) {
            if ( in_array( $custom_menu_item[2], $removed_menu_items ) ) {
                unset( $custom_menu[$index] );
            }
        }
    }
}

Answer 1

There is nothing inherently wrong with foreach loops.

Alternatives might include:

• Creating objects that contain the data and extend Iterator, and using that to recurse the options. The disadvantages are that it would be less efficient and would involve quite a lot more code. The advantage is it would make the foreach loops more readable; particularly whats inside them, once you give methods to each of the objects that extend Iterator that perform the action that happens within the foreach code.
• use PHP array functions. This is probably the most efficient, but can make code painfully unreadable. I would not recommend this unless you desperately need greater efficiency.

By the way, one change I would make no matter what you do, is replace "magic constants" with named constants. E.g.:

if ( in_array( $menu_item[2], $new_menu_items ) ) {

becomes

define ('MEANINGFUL_NAME', 2);

// ....

if ( in_array( $menu_item[MEANINGFUL_NAME], $new_menu_items ) ) {

Otherwise, when you come back to look at the code after a long time, you will have no idea if each time you see a 2, whether they all refer to the same "thing" or not; in other words, say you want to change an aspect of it to be "3" instead, you will have to painstakingly work out what each bit of code does to see if it is a 2 that you need to change or not. (Using comments is a bad idea for this, as comments are easily forgotten to be changed, and it would have to be commented each time you have a magic number - the more you have, the more likely changes in code will not have corresponding comments changed

One final change I would make is perhaps splitting up the function into sub-functions, since doing that would mean you can give the functions names that describe the sub-process being performed. Putting that method and sub methods into an appropriate class could nicely tidy things up. While OOP can lead (IMO) to the very best code, if it's implemented badly it can be a nightmare to manage. Plenty of books on it, which I could recommend if you like. I have 5-10 books on it that I always keep by my side.

• "Design Patterns" Gamma / Helm / Jhnson / Vlissides (1995)
• "Patterns of Enterprise Application Architecture" Martin Fowler (2003)
• "The pragmatic programmer" Hunt / Thomas (2000)
• "Refactoring to patterns" Kerievsky (2005)
• "Refactoring" - Martin Fowler (1999)
• "OO Design Heuristics" - Arthur Riel (1996)
• "Growing Object-Oriented Software Guided by Tests" - Freeman / Pryce (2009)

I would actually suggest first getting Freeman/Pryce, even if you find TDD doesn't really agree with you - their bibliography was very useful to me, and I picked and choose what suited me. There's a lot of controversy about TDD, but they'll show you why refactoring is such a powerful principle, and you can then try one or two books on patterns and refactoring. Choose one that seems to have patterns you think you'll be most likely to use.