patternphplaravelMinor
Controller to manage athletes
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
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,
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.