MVC controller and action that handles favorite items in a session-based list

Question

I've written a Controller and a corresponding Action for a Symfony 2.8 project, that does the following:

• handles a list of favorite items of a user
• the ids of the favorites are stored in the session
• multiple entites can be favorized
• possible actions on the favorite list are [add|remove]
• the controller's action is called via AJAX and returns a JSON object

---

My questions are:

• Can the PHP Code be optimized?
• Is this code DRY?
• Is this code safe?
• Is this too much logic for one action?
• Is it too much or not enough or even wrongly documented?

---

A few notes:

• $this->getRepositoryName() is a custom function from the AbstracController base class that returns the correct repository name for a given entity
• the returned JSON object is more then just the state of success, so the JavaScript can see, what exactly failed, if one ore more properties come back empty
• I only need the favorites in the session for a filter later, they are not going to be stored or tracked using a database

---

class FavoritesController extends AbstractController
{
    private $validEntites = ['entityA', 'entityB'];
    private $validActions = ['add', 'remove'];

    /**
     * Can add or remove favorites to/from the list in the session.
     *
     * @Route("/handle-favorites", name="handle_favorites")
     *  
     * @param Request $request
     * @return \Symfony\Component\HttpFoundation\JsonResponse
     */
    public function handleFavoritesAction(Request $request)
    {
        // initiate return value, will be send as JSON
        $result = [
            'success' => true,
            'entity' => null,
            'count' => null,
            'id' => null,
            'action' => null,
        ];

        // test for XHR request
        if(!$request->isXmlHttpRequest()) {
            $result['success'] = false;
            return new JsonResponse($result);
        }

        // retrieve values from session and create favorites array if necessary
        $session = $request->getSession();
        $favorites = $session->get('favorites');

        if (!$favorites) {
            $favorites = [];

            foreach ($this->validEntites as $value) {
                $favorites[$value] = [];
            }
        }

        // get and test request parameters 
        $entity = $request->get('entity');
        $id = $request->get('id');
        $action = $request->get('action');

        if (in_array($entity, $this->validEntites)) {
            $result['entity'] = $entity;
        } else {
            $result['success'] = false;
        }

        if (in_array($action, $this->validActions)) {
            $result['action'] = $action;
        } else {
            $result['success'] = false;
        }

        if (false !== filter_var($id, FILTER_VALIDATE_INT, array('options' => array('min_range'=> 1)))) {
            $result['id'] = $id;
        } else {
            $result['success'] = false;
        }

        // add or remove item
        if ($result['success']) {
            if ('remove' === $action) {
                $favorites[$entity] = array_diff($favorites[$entity], [$id]);
            }

            if ('add' === $action && !in_array($id, $favorites[$entity])) {
                $repository = $this->getDoctrine()->getRepository($this->getRepositoryName(ucfirst($entity)));
                $object = $repository->find($id);

                if ($object) {
                    $favorites[$entity][] = $id;
                } else {
                    $result['success'] = false;
                    $id = null;
                    $result['id'] = $id;
                }
            }

            // update session
            $session->set('favorites', $favorites);
        }

        // update count
        $result['count'] = count($favorites[$entity]);           

        return new JsonResponse($result);
    }
}

Answer 1

A few thoughts.

• Consider building a result or response class to:
  • provide better structure around the returned result
  • provide possible re-use in other controllers
  • and clean up this code.

For example:

// instantiation
$result = new Result();

// usage
$result->setSuccess(true);
$result->setEntity($entity);

You could have this class implement JsonSerializable interface so you can pass it directly to JsonResponse() for serialization. This also allows you to do things like add validation that setters are passed valid values (i.e. boolean for setSuccess())

• You have a number of cases where you could remove else blocks and code nesting if you invert your conditionals.

For example:

    if (in_array($entity, $this->validEntites)) {
        $result['entity'] = $entity;
    } else {
        $result['success'] = false;
    }

can become:

   $result['success'] = false; 
   if (in_array($entity, $this->validEntites)) {
        $result['entity'] = $entity;
   }

or:

  if(!in_array($entity, $this-.validEntities)) {
      $result['success'] = false;
      return JsonResponse($result);
  }
  $result['entity'] = $entity;

Similarly:

if($result['success']) {
    // nested code
}

could become:

if(!$result['success']) {
    return JsonResponse($result);
}
// rest of code, now without nesting

Look for opportunities to remove code paths and complex nesting by getting rid of unnecessary else blocks (many of which truly are unnecessary) and exiting early by inverting conditions. The fewer code paths you have in your code, the less fragile, easier to test, and easier to maintain it will be.

• I think if you clean those things up, you will probably save 40% of the lines of code in this method. So it might not look like there is "too much" happening in this single method. That being said, I think you could possibly fork the execution path along the lines of $action value , and hand off to protected/private class methods to finish that particular work.

For example:

switch($action) {
    case 'remove':
        return $this->removeFavorite(...);
    case 'add':
        return $this->addFavorite(...);
    default:
        $result['success'] = false;
        return JsonResponse($result);
} 

This would allow to more easily unit test those individual methods that represent some subset of logic on dealing with "favorites".

• Similarly, you could perform all your upfront Request validation in a separate method as well to where you are getting towards something more like this in your main method.

Ex:

public function handleFavoritesAction(Request $request) {
    // here $result could be Result object as suggested above
    $result = $this->validateRequest($request);
    if($result->isSuccess === false) {
       return JsonResponse($result);
    }
    $action = $request->get('action');
    switch ($action) {
        // switch as shown above
    }
}

• With regards to your documentation, I find many of your comments to be unnecessary. I think you generally have meaningfully named variables and appropriate use of vertical whitespace between logical sections of code to where comments become superfluous. Especially, if you consider breaking this main method into several other well-named methods, you should not have much need to comment this code.