HiveBrain v1.2.0
Get Started
← Back to all entries
patternphplaravelMinor

Controller to manage athletes

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
athletesmanagecontroller

Problem

I would appreciate if you could give me some advice about the code below. I am a student learning PHP programming and programming fundamentals. I have learned a lot from Stack Overflow but there are lot of things I don't know. I would like a senior developer's opinion on improving my codes overall quality.

This is a part of my API code. I wrote this using Laravel framework. I want to know how to refactor my code using repositories and service layers.

```
use Athlete\Requests\PlayerRequest;
use Sorskod\Larasponse\Larasponse;
use Athlete\Transformers\PlayerTransformer;
use Athlete\Repositories\Player\PlayerRepository;

class PlayersController extends ApiController {

/**
* @var \Sorskod\Larasponse\Larasponse $fractal
*/
private $fractal;

/**
* @var \Athlete\Repositories\Player\PlayerRepository $repository
*/
private $repository;

/**
* @var \Athlete\Requests\PlayerRequest $playerRequest
*/
private $playerRequest;

/**
* @param \Sorskod\Larasponse\Larasponse $fractal
* @param PlayerRepository $repository
* @param \Athlete\Requests\PlayerRequest $playerRequest
*/
public function __construct(Larasponse $fractal,
PlayerRepository $repository,
PlayerRequest $playerRequest
)
{
$this->fractal = $fractal;
$this->fractal->parseIncludes($this->getIncludes());

$this->repository = $repository;
$this->playerRequest = $playerRequest;
}

/**
* Display a listing of the resource.
* GET /players
*
* @param $sportId
* @param $teamId
* @return \Response
*/
public function index($sportId, $teamId)
{
$limit = Request::get('limit') ?: 20;

$offset = Request::get('offset') ?: 0;

$team = Auth::user()->sports()->findOrfail($sportId)->teams()->findOrFail($teamId);

$players = $this->repository->filterByTeam($team->id)->pagin

Solution

You are putting way too much logic into your controller methods. Most of the logic should be placed in a service that should be injected into the controller.

The logic in the controller methods really shouldn't do much more than grab a value from here, set a value there and maybe call a function on a service.

The most extreme way of achieving this would be to make the entire controller a service, creating a new controller and see how much you need to move back from the service into the controller.

In regards to the "how", you may want to take a look at Chris Fidao's talk about hexagonal-architecture,

Context

StackExchange Code Review Q#88050, answer score: 2

Revisions (0)

No revisions yet.