Return to Answer

added 9 characters in body

Source Link

edited May 20, 2017 at 16:08

Mike Brant

9.9k
14
24

Consider building a result or response class to:

provide more better structure around the returned result,

provide possible re-use in other controllers,

and clean up this code.

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

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. This 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 ofoff to protected/private class methodmethods to finish that particular work.

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

With regards to your documentation, I find many of your comments to be unnecessary. I think you generally have meaningfully named 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.

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

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

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. This 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 of to protected/private class method to finish that particular work.

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

With regards to your documentation, I find many of your comments to be unnecessary. I think you generally have meaningfully named 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.

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.

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())

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.

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

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.

Source Link

answered May 18, 2017 at 17:26

Mike Brant

9.9k
14
24

A few thoughts.

Consider building a result or response class to provide more 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 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. This 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 of to protected/private class method to finish that particular work.

For example:

switch($action) {
    case 'remove':
        return $this->removeFavorite(...);
        break;
    case 'add':
        return $this->addFavorite(...);
        break;
    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 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.