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:

1. User
2. Role
3. 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:

1. Menu
2. MenuItem
3. 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:

1. get all of the user's roles by the given userId
2. get the menu id by the given menuName
3. get the menu item id by the given menuItemName
4. get all of the menu item roles by the results of #2 and #3
5. 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.

• Tim Morton: 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.

Answer 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));