Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I was hoping someone could take a look at my users controller and give their feedback based one their opinions of what they think of my user's controller. As well as maybe give some ideas if any for some of the current TODO's in my function phpdocs.

<?php namespace App\Http\Controllers;

use App\Repositories\Users\UserRepository;
use App\Repositories\UserRoles\UserRoleRepository;
use App\Repositories\UserProfiles\UserProfileRepository;
use App\Http\Requests;
use App\Http\Requests\UserRequest;
use Illuminate\HttpResponse;
use App\Http\Controllers\Controller;

class UsersController extends Controller {

    private $userRepository;
    private $userRoleRepository;
    private $userProfileRepository;

    /**
     * @param UserRepository        $userRepository
     * @param UserRoleRepository    $userRoleRepository
     * @param UserProfileRepository $userProfileRepository
     */
    public function __construct(UserRepository $userRepository, UserRoleRepository $userRoleRepository, UserProfileRepository $userProfileRepository)
    {
        $this->userRepository = $userRepository;
        $this->userRoleRepository = $userRoleRepository;
        $this->userProfileRepository = $userProfileRepository;
    }

    /**
     * Display a listing of the users.
     *
     * TODO: Partial the edit, delete, show anchor icons in view.
     *
     * @return Response
     */
    public function index()
    {
        $users = $this->userRepository->with('role')->getAll();

        return view('users.index', compact('users'));
    }

    /**
     * Show the form for creating a new user.
     *
     * TODO: Fix the coding of the form so that its easier to handle.
     *
     * @return Response
     *
     */
    public function create()
    {
        $userRoles = $this->userRoleRepository->getForSelect('name');

        return view('users.create', compact('userRoles'));
    }

    /**
     * Save a new user and the user's profile.
     *
     * @param UserRequest $request
     *
     * TODO: Find easier way to handle creation of a profile with a user since relationship is set up in User's model.
     *
     * @return Response
     */
    public function store(UserRequest $request)
    {
        $user = $this->userRepository->create($request->all());

        $user_id = $user->id;

        $this->userProfileRepository->create([
            'user_id' => $user_id
        ]);

        return redirect('users');
    }

    /**
     * Display the specified resource with their profile.
     *
     * @param  int $id
     *
     * @return Response
     */
    public function show($id)
    {
        $user = $this->userRepository->with('profile')->getById($id);

        return view('users.show', compact('user'));
    }

    /**
     * Show the form for editing the specified resource.
     *
     * @param  int $id
     * TODO: Find out how to handle password field if not editing password for user.
     *
     * @return Response
     */
    public function edit($id)
    {
        $user = $this->userRepository->getById($id);

        $userRoles = $this->userRoleRepository->getForSelect('name');

        return view('users.edit', compact('user', 'userRoles'));
    }

    /**
     * Update the specified resource from storage.
     *
     * @param  int        $id
     * @param UserRequest $request
     *
     * TODO: If updated user is currently logged in do I need to destroy session to force them to log back in case role/permission or other data is changed.
     * TODO: Fix not updating user properly.
     *
     * @return Response
     */
    public function update($id, UserRequest $request)
    {
        $user = $this->userRepository->getById($id);

        $this->userRepository->update($request->all(), $user->id);

        return redirect('users');
    }

    /**
     * Remove the specified resource from storage.
     *
     * @param  int $id id
     *
     * TODO: Find out if I can handle this differently since relationship exists between a user and a profile as stated in User model.
     *
     * @return Response
     */
    public function destroy($id)
    {
        $this->userRepository->delete($id);

        $this->userProfileRepository->delete($id);

        return redirect('users');
    }

}
share|improve this question
    
As far as controllers go it is coded well for unit testing / spec testing. In reality this actually means that you've written a dumb class, it has no substantial logic other than "use x to accomplish y and return the results of y = x() as z(y)". There isn't much more to review here so I feel uncomfortable providing a critical answer where the majority of the logic is in the repositories. –  David Barker Apr 23 at 11:23

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Browse other questions tagged or ask your own question.