Be part of JetBrains PHPverse 2026 on June 9 – a free online event bringing PHP devs worldwide together.

lkmadushan's avatar

Need advices for writing better code in Laravel

I am very appreciate if you can give me some advises after looking the code below. I am a students and learning PHP programming and programming foundation. I learned very much things from Laravel Community and Jeffery's tutorials. There are lot of things i have to learn still. I like to know senior developer's advises to improve my code quality. Thanks.

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);
        }
    }
}
0 likes
9 replies
davorminchorov's avatar

You can refactor this code and move those implementations into readable methods or service classes.

1 like
lkmadushan's avatar

@Ruffles Thanks for your advice. So if i use repositories what should i put it from the above mentioned class.

martinbean's avatar

@lkmadushan A tiny improvement: the second parameter of the Request::get() method can be a default value, so you can replace these ternary statements:

$limit = Request::get('limit') ?: 20;

With something like this:

$limit = Request::get('limit', 20);

However, I would just use Laravel’s built-in paginator instead of hard-coded these “magic” numbers in your controller methods.

2 likes
lkmadushan's avatar

@martinbean Thank you very much :) That is a nice improvement and i should reduce magic numbers as you said. Thanks again.

dixitchopra's avatar

@Ruffles I am using Services in my constructor class and then services call repositories. I am putting required Service classes in _construct method and required repositories in Service's _construct method. Is that the correct way of doing it?

davorminchorov's avatar

@dixitchopra Yeah, that's good because you will use the repositories in the service classes and the service classes will be used in the controller class.

dixitchopra's avatar

@Ruffles I am writing required repositories in service class's _construct method. I hope, that won't cause any performance issues. Sometimes, it's like three to four repositories in one service class.

davorminchorov's avatar

@dixitchopra I don't think there's any performance difference with it. It mainly for developers and easier maintenance for the long run.

As for having 3-4 repositories injected into a constructor, it should be fine but be careful not to get more than that. Read up more on Object Oriented Design and some of those rules for more info.

Please or to participate in this conversation.