3

How should I structure a piece of code that executes an operation, but may have slightly different behavior depending on, let's say, user roles?

Example: My app has a 'manager' and a 'employee' roles. There are many managers in my app, and each of them have many employees. I have a dashboard where managers and their employees can add/edit/delete products.

Both can edit the products the have created themselves, but managers can edit products belonging to his employees. If a manager edits a product created by an employee, the employee will get an email notification about the edit.

The issue I have, is that I´m creating if/else or switch/case statements for checking the user roles, and I feel once I add new roles my code will need to check for them too and my code will become harder to read.

For example:

// What I currently have:
public function updateProductForUser(Product $product, UserInterface $user)
{
    $userWhoCreatedProduct = $product->getCreatedByUser();

    if ($userWhoCreatedProduct === $user) {
        // If the user who created the product is the same one trying to update it,
        // then go ahead and execute the update.
        $this->_doUpdateProductForUser($product, $user);
        return;

    } elseif ($user instanceof Manager) {
        $allManagerEmployees = $this->someService->findAllEmployeesOfManager($user);

        if (in_array($userWhoCreatedProduct, $allManagerEmployees)) {
            // If the user trying to update the product is a manager, and the product was
            // created by an employee of the manager, then execute the update but also
            // send an notification to the employee that the product he created got updated
            // by his manager.
            $this->_doUpdateProductForUser($product, $user);
            $this->notifyUserThatProductGotUpdated($product);
        }
    }
}

How can I improve this? Is there a better way? What are the pitfalls of the implementation? Any advice is very welcomed. Thanks!

  • Please explain with words, not code. – Tibo Jan 23 '16 at 21:36
  • Are you asking about architecture or source code. The latter is better for stack overflow and is off-topic here; architecture might be on topic. – Tom Au Jan 23 '16 at 21:46
  • @Tibo, Tom Au. Many thanks for responding. I´ve removed most of the code and left the important bits. My question is about architecture and avoid my code from getting messier. Thanks! – Oscar Balladares Jan 24 '16 at 22:47
7

You can make the code a little more maintainable if you

  • Put the product handling logic (without permission logic and without other side effects) into a separate service class ProductDomain. (See Separation of concerns for details)
  • Create a ProductHandlingService class that uses ProductDomain and applies permissions and other side effects
  • give the conditions a name (See Strategy pattern for details)

The ProductHandlingService might look like this:

public function updateProductForUser(Product $product, UserInterface $userWhoWantsToModify)
{
  if ($strategy->userIsAllowedToModifyProduct($product, $userWhoWantsToModify) {
    $ProductDomain->_doUpdateProductForUser($product, $userWhoWantsToModify);
    if ($strategy->userNeedsInfoAboutProductUpdate($product, $product->getCreatedByUser(),
          $userWhoWantsToModify) {
      $this->notifyUserThatProductGotUpdated($product);
    }
  }
}

Advantages over the original design:

  • core-ProductDomain, Permission-detection and special-handling can be tested independently.
  • During software lifetime it is likely that Permission-detection and special-handling will be change often due to political reasons.
  • core-ProductDomain is likely to change not very often and mostly because of technical reasons.

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.