1
\$\begingroup\$

I have been trying to implement good 'OOP' and coding standards, but I always end up with bloated controllers. How could I clean this controller method?

public function updatePositions(Request $request)
{
    MenuItem::setNewOrder($request->positions);
    $item = MenuItem::findOrFail($request->itemId);

    if (!is_array($request->path)) {
        return response()->json('Invalid data', 400);
    }

    if (count($request->path) === 1) {
        if ($item->parent()->exists()) {
            $item->parent()->dissociate();
            $item->save();
        }
    } else {
        $potentialParent = $item->sort_order - last($request->path) - 1;
        $previousRecord = MenuItem::where('sort_order', $potentialParent)->first();
        $item->parent()->associate($previousRecord);
        $item->save();
    }
}
  • it sets a completely new order for the MenuItem at hand, which I think in itself my be a method?

  • it checks if the $request->path is an array and returns a formatted json response, should this throw an exception?

  • It then checks the length of $request->path and disassociates a relation model or if the length is greater then 1, it associates a relationship object after finding that.

Any advice in the good direction is welcome here.

\$\endgroup\$
  • \$\begingroup\$ More context would be useful. Could you clarify what exactly is a Request and a MenuItem? What class does this method appear in, and what else is in the class? \$\endgroup\$ – 200_success Mar 15 at 16:53
  • 1
    \$\begingroup\$ How many times do you check this return response()->json('Invalid data', 400); - I am not a Laravel user, but I am a PHP user for many years... It may be possible to put it in the constructor, that's why I ask. But I don't know the best way to get $request that seems like something very important to a controller. I was just thinking - "I wonder if they check that in every/many method(s)" etc. In any case you should do a bit sooner so do your $item=MenuItem::findOrFail($request->itemId); after this then you don't waste that ORM call if that return is triggered. \$\endgroup\$ – ArtisticPhoenix Mar 15 at 21:32
  • 1
    \$\begingroup\$ MenuItem::setNewOrder($request->positions); like I said above this one looks like its setting something, so you may not want to do that if !is_array($request->path). But I don't know, maybe you do. It just seemed suspect to this non Laravel user. For example why would you save that if you issue this 'Invalid data', 400 in your JSON response. But it may not even be percipient, so it's just a wasted call. . If you follow. \$\endgroup\$ – ArtisticPhoenix Mar 15 at 21:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Browse other questions tagged or ask your own question.