PHP Optimizing Nested Loop

Question

Admittedly, I'm not very strong with PHP. I have a situation where I need to check if a given user is authorized to visit a given webpage.

A user can potentially have zero or more roles. To do this, I setup three tables:

User
Role
UserRole

Role references user. UserRole references both User and Role.

A webpage is will always have a parent menu to reference. Think of it in the case of a dropdown menu where the top-level menu item does not represent a webpage, but the child menu items do.

To do this, I setup 3 tables:

Menu
MenuItem
MenuItemRole

Menu and MenuItem both have a name. MenuItem references Menu. And MenuItemRole references both Menu and MenuItem.

Now I created a function that has the following arguments: menuName, menuItemName, and userId.

As far as my business logic is concerned, I am doing the following:

get all of the user's roles by the given userId
get the menu id by the given menuName
get the menu item id by the given menuItemName
get all of the menu item roles by the results of #2 and #3
loop over all the records from #1 and #4 until a match is found

This is the code that I setup:

public static function authorizeView($menuName, $menuItemName, $userId) {
    if (!$menuName || $menuItemName || $userId) {
        throw new Exception("Unauthorized");
    }

    // get all of the user's roles
    $filters = array();
    $filters["UserId"] = $userId;
    $userRoles = UserRoleService::query($filters);
    if ($userRoles["total"] === 0) {
        return false;
    }

    // get the menu id
    $filters = array();
    $filters["MenuName"] = $menuName;
    $menus = MenuService::query($filters);
    if ($menus["total"] === 0) {
        return false;
    }
    $menuId = $menus["records"][0]["MenuId"];

    // get the menu item id
    $filters = array();
    $filters["MenuItemName"] = $menuItemName;
    $menuItems = MenuItemService::query($filters);
    if ($menuItems["total"] === 0) {
        return false;
    }
    $menuItemId = $menuItems["records"][0]["MenuItemId"];

    // get the menu item roles by the menu/menu item
    $filters = array();
    $filters["MenuId"] = $menuId;
    $filters["MenuItemId"] = $menuItemId;
    $menuItemRoles = MenuItemRoleService::query($filters);
    if ($menuItemRoles["total"] === 0) {
        return false;
    }

    // loop over all of the user roles, if one of the values is in one of the menu item roles, return true
    $authorized = false;
    foreach ($userRoles as $userRole) {
        foreach ($menuItemRoles as $menuItemRole) {
            if ($userRole["RoleId"] === $menuItemRole["RoleId"]) {
                $authorized = true;
                break;
            }
        }
        if ($authorized) {
            break;
        }
    }

    // otherwise return false
    return $authorized;
}

My first question is: am I architect-ing this right or is it over-engineered?

My second question is: can the nested loop be optimized for readability? Up to that point, I can read everything pretty clearly, but that one section sticks out at me because it is not immediately clear what it is doing.

This would probably be more appropriate on code review, if this code works. As for readability, it could use much improvement, starting with SRP. This method does too much, as noted in your comments. — Tim Morton
– Tim Morton, Commented Feb 3, 2021 at 5:41

Kien Nguyen · Accepted Answer · 2021-02-03 04:42:51Z

1

About the first question: It is difficult for me to judge if your architecture is absolutely right or not. As that depends on other logic also related to role/menu (if any). But basically I find it quite similar to the architectures I usually come across, so I don't think there's a problem here.

About the second question: Your nested loop can be rewritten as taking the intersection of 2 arrays. Then, the logic would be: "if the user has at least one role associated with the menu and menu item, he will be authorized". I think that's a bit easier to read.

I rewrote your code (from line // loop over all...) following the above approach:

$userRoleIds = array_column($userRole, "RoleId");
$menuItemRoleIds = array_column($menuItemRoles, "RoleId");
return !empty(array_intersect($userRoleIds, $menuItemRoleIds));

answered Feb 3, 2021 at 4:42

Kien Nguyen

2,7212 gold badges9 silver badges25 bronze badges

Sign up to request clarification or add additional context in comments.

Collectives™ on Stack Overflow

PHP Optimizing Nested Loop

1 Answer 1

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

Comments

Your Answer

Sign up or log in

Post as a guest

Related