6
\$\begingroup\$

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)->paginate($limit, $offset);

        $data = $this->fractal->collection($players, new playerTransformer(), 'players');

        return $this->respondWithSuccess($data);
    }

    /**
     * Store a newly created resource in storage.
     * POST /sports
     *
     * @param $sportId
     * @param $teamId
     * @return \Response
     */
    public function store($sportId, $teamId)
    {
        $formData = Input::all();

        $this->playerRequest->validate($formData);

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

        try {
            DB::beginTransaction();

            if(Input::hasFile('image')) {
                $formData = array_merge($formData, ['image' => Str::random()]);
            }

            $player = $team->players()->create($formData);

            //save weight, height of the player if they exists
            if(array_key_exists('weight_unit', $formData)) {

                $player->weight()->save(new Weight([
                    'unit' => $formData['weight_unit'],
                    'value' => $formData['weight_value']
                ]));
            }

            if(array_key_exists('height_unit', $formData)) {

                $player->height()->save(new Height([
                    'unit' => $formData['height_unit'],
                    'value' => $formData['height_value']
                ]));
            }

            $this->moveImage($player->id, $player->image);

            DB::commit();
        } catch(Exception $e) {
            DB::rollBack();

            return $this->respondUnprocess($e->getMessage());
        }

        $data = $this->fractal->item($player, new PlayerTransformer());

        return $this->respondWithSuccess(array_merge($data, ['players_count' => Player::count()]));
    }

    /**
     * Display the specified resource.
     * GET /sports/{id}
     *
     * @param $sportId
     * @param $teamId
     * @param $playerId
     * @return \Response
     */
    public function show($sportId, $teamId, $playerId)
    {
        $team = Auth::user()->sports()->findOrFail($sportId)->teams()->findOrFail($teamId);

        $player = $team->players()->findOrFail($playerId);

        $data = $this->fractal->item($player, new playerTransformer());

        return $this->respondWithSuccess($data);
    }

    /**
     * Update the specified resource in storage.
     * PUT /players/{playerId}
     *
     * @param $sportId
     * @param $teamId
     * @param $playerId
     * @return \Response
     * @throws \Laracasts\Validation\FormValidationException
     */
    public function update($sportId, $teamId, $playerId)
    {
        $formData = Input::all();

        $this->playerRequest->updateRules()->validate($formData);

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

        $player = $team->players()->findOrFail($playerId);

        try {
            DB::beginTransaction();

            // rename the image name to clear caching for mobile devices
            if(Input::hasFile('image')) {
                $path = storage_path("players/{$player->id}/{$player->image}");
                File::delete($path);

                $formData = array_merge($formData, ['image' => Str::random()]);
            }

            $player->update($formData);

            //update weight, height of the player
            if(array_key_exists('weight_unit', $formData)) {

                Weight::updateOrCreate([
                    'id' => $playerId
                ], [
                    'unit' => $formData['weight_unit'],
                    'value' => $formData['weight_value']
                ]);
            }

            if(array_key_exists('height_unit', $formData)) {

                Height::updateOrCreate([
                    'id' => $playerId
                ], [
                    'unit' => $formData['height_unit'],
                    'value' => $formData['height_value']
                ]);
            }

            $this->moveImage($player->id, $player->image);

            DB::commit();
        } catch(Exception $e) {
            DB::rollBack();

            return $this->respondUnprocess('Unable to update the player!');
        }

        $data = $this->fractal->item($player, new PlayerTransformer());

        return $this->respondWithSuccess($data);
    }

    /**
     * Remove the specified resource from storage.
     * DELETE /players/{playerId}
     *
     * @param $sportId
     * @param $teamId
     * @param $playerId
     * @return \Response
     */
    public function destroy($sportId, $teamId, $playerId)
    {
        $team = Auth::user()->sports()->findOrFail($sportId)->teams()->findOrFail($teamId);

        $player = $team->players()->findOrFail($playerId);

        try {
            if($player->image != 'default.png') {
                $path = storage_path("players/{$player->id}");

                $player->delete();
                File::delete($path);
            }
        } catch(Exception $e) {

            return $this->respondUnprocess('Unable to delete the player!');
        }

        return $this->respondWithSuccess('Player has been successfully deleted.');
    }

    /**
     * Move image to app/storage path
     *
     * @param $path
     * @param $name
     */
    private function moveImage($path, $name)
    {
        if(Input::hasFile('image') && Input::file('image')->isValid()) {

            Input::file('image')->move(storage_path("players/$path"), $name);
        }
    }
}
\$\endgroup\$
5
  • 3
    \$\begingroup\$ Welcome to Code Review! To make life easier for reviewers, please add sufficient context to your question. The more you tell us about what your code does and what the purpose of doing that is, the easier it will be for reviewers to help you. See also this meta question \$\endgroup\$
    – Vogel612
    Commented Apr 26, 2015 at 17:42
  • 3
    \$\begingroup\$ So, is this part of a game? A sports simulation of some kind? Please add more info about what this code does. \$\endgroup\$
    – glampert
    Commented Apr 26, 2015 at 19:12
  • \$\begingroup\$ This a part of Athlete Management Software. It can manage players, teams, sports, videos and skills. This is the PlayersController. \$\endgroup\$ Commented Apr 27, 2015 at 6:26
  • \$\begingroup\$ Also could you provide the code from the ApiController. I can see calls to methods such as getIncludes(), which I assume are declared inside the parent class. \$\endgroup\$
    – AnotherGuy
    Commented Apr 27, 2015 at 18:29
  • \$\begingroup\$ I'm guessing by "managing players, teams, sports and skills" you mean that it collects data to define such items and allows those items to be edited in the event of changes. Is that correct? \$\endgroup\$
    – A Smith
    Commented Jul 8, 2015 at 10:40

1 Answer 1

2
\$\begingroup\$

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,

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.