Controller for repository of members

Question

I tried to implement the Repository pattern for my project:

MemberController.php -- this one is a bit too long and has repetitive code

<?php

class MemberController extends \BaseController {

    private $repo;

    public function __construct(MemberRepository $repo)
    {
        $this->repo = $repo;
    }

    //////////////////////
    // 'Simple' pages   //
    //////////////////////

    public function index()
    {
        $yourId = Auth::user()->id;
        $data = $this->repo->getChildren($yourId);
        return View::make('members.index', $data);
    }

    public function view($id)
    {
        $yourId = Auth::user()->id;
        if ($this->repo->canView($yourId, $id)) {
            $data = $this->repo->getDetails($id);
            return View::make('members.view', $data);
        } else {
            return Response::make('Unauthorized', 401);
        }
    }

    public function ledger($id)
    {
        $yourId = Auth::user()->id;
        if ($this->repo->canView($yourId, $id)) {
            $data = $this->repo->getLedger($id);
            return View::make('members.ledger', $data);
        } else {
            return Response::make('Unauthorized', 401);
        }
    }

    public function log($id)
    {
        $yourId = Auth::user()->id;
        if ($this->repo->canView($yourId, $id)) {
            $data = $this->repo->getLog($id);
            return View::make('members.log', $data);
        } else {
            return Response::make('Unauthorized', 401);
        }
    }

    //////////////////////
    // Show Forms (GET) //
    //////////////////////

    public function showCreate()
    {
        return View::make('members.create');
    }

    public function showEdit($id)
    {
        $yourId = Auth::user()->id;
        if ($this->repo->canModify($yourId, $id)) {
            $data = $this->repo->getInfo($id);
            return View::make('members.edit', $data);
        } else {
            return Response::make('Unauthorized', 401);
        }
    }

    public function showBan($id)
    {
        $yourId = Auth::user()->id;
        if ($this->repo->canModify($yourId, $id)) {
            $data = $this->repo->getInfo($id);
            return View::make('members.ban', $data);
        } else {
            return Response::make('Unauthorized', 401);
        }
    }

    public function showUnBan($id)
    {
        $yourId = Auth::user()->id;
        if ($this->repo->canModify($yourId, $id)) {
            $data = $this->repo->getInfo($id);
            return View::make('members.unban', $data);
        } else {
            return Response::make('Unauthorized', 401);
        }
    }

    public function showDelete($id)
    {
        $yourId = Auth::user()->id;
        if ($this->repo->canModify($yourId, $id)) {
            $data = $this->repo->getInfo($id);
            return View::make('members.delete', $data);
        } else {
            return Response::make('Unauthorized', 401);
        }
    }

    public function showTopup($id)
    {
        $yourId = Auth::user()->id;
        if ($this->repo->canModify($yourId, $id)) {
            $data = $this->repo->getInfo($id);
            return View::make('members.topup', $data);
        } else {
            return Response::make('Unauthorized', 401);
        }
    }

    public function showWithdraw($id)
    {
        $yourId = Auth::user()->id;
        if ($this->repo->canModify($yourId, $id)) {
            $data = $this->repo->getInfo($id);
            return View::make('members.withdraw', $data);
        } else {
            return Response::make('Unauthorized', 401);
        }
    }

    public function showReset($id)
    {
        $yourId = Auth::user()->id;
        if ($this->repo->canModify($yourId, $id)) {
            $data = $this->repo->getInfo($id);
            return View::make('members.reset', $data);
        } else {
            return Response::make('Unauthorized', 401);
        }
    }

    ///////////////////////
    // Do Actions (POST) //
    ///////////////////////

    public function doCreate()
    {
        $yourId = Auth::user()->id;
        $result = $this->repo->create($yourId, Input::all());
        if ($result->success) {
            return Redirect::route('members.index')->with('message', $result->message);
        } else {
            return Redirect::back()->withErrors($result->message);
        }
    }

    public function doEdit($id)
    {
        $yourId = Auth::user()->id;
        if ($this->repo->canModify($yourId, $id)) {
            $result = $this->repo->edit($id, Input::all());
            if ($result->success) {
                return Redirect::route('members.index')->with('message', $result->message);
            } else {
                return Redirect::back()->withErrors($result->message);
            }
        } else {
            return Response::make('Unauthorized', 401);
        }
    }

    public function doBan($id)
    {
        $yourId = Auth::user()->id;
        if ($this->repo->canModify($yourId, $id)) {
            $result = $this->repo->ban($id);
            if ($result->success) {
                return Redirect::route('members.index')->with('message', $result->message);
            } else {
                return Redirect::back()->withErrors($result->message);
            }
        } else {
            return Response::make('Unauthorized', 401);
        }
    }

    public function doUnBan($id)
    {
        $yourId = Auth::user()->id;
        if ($this->repo->canModify($yourId, $id)) {
            $result = $this->repo->unban($id);
            if ($result->success) {
                return Redirect::route('members.index')->with('message', $result->message);
            } else {
                return Redirect::back()->withErrors($result->message);
            }
        } else {
            return Response::make('Unauthorized', 401);
        }
    }

    public function doDelete($id)
    {
        $yourId = Auth::user()->id;
        if ($this->repo->canModify($yourId, $id)) {
            $result = $this->repo->delete($id);
            if ($result->success) {
                return Redirect::route('members.index')->with('message', $result->message);
            } else {
                return Redirect::back()->withErrors($result->message);
            }
        } else {
            return Response::make('Unauthorized', 401);
        }
    }

    public function doTopUp($id)
    {
        $yourId = Auth::user()->id;
        if ($this->repo->canModify($yourId, $id)) {
            $result = $this->repo->topUp($id, Input::only('amount'));
            if ($result->success) {
                return Redirect::route('members.index')->with('message', $result->message);
            } else {
                return Redirect::back()->withErrors($result->message);
            }
        } else {
            return Response::make('Unauthorized', 401);
        }
    }

    public function doWithdraw($id)
    {
        $yourId = Auth::user()->id;
        if ($this->repo->canModify($yourId, $id)) {
            $result = $this->repo->withdraw($id, Input::only('amount'));
            if ($result) {
                return Redirect::route('members.index')->with('message', $result->message);
            } else {
                return Redirect::back()->withErrors($result->message);
            }
        } else {
            return Response::make('Unauthorized', 401);
        }
    }

    public function doReset($id)
    {
        $yourId = Auth::user()->id;
        if ($this->repo->canModify($yourId, $id)) {
            $result = $this->repo->resetPasswordAndPin($yourId, $id, Input::all());
            if ($result) {
                return Redirect::route('members.index')->with('message', $result->message);
            } else {
                return Redirect::back()->withErrors($result->message);
            }
        } else {
            return Response::make('Unauthorized', 401);
        }
    }

}

MemberRepository.php -- this one becomes a large class, with all code removed except for the function names; just wanted to show that it has many functions

<?php

class MemberRepository
{
    /////////////////
    // Permissions //
    /////////////////

    public function canView($viewerId, $memberId)
    {
        return true;
    }

    public function canModify($viewerId, $memberId)
    {
        return true;
    }

    //////////////////
    // Getting Data //
    //////////////////

    public function getChildren($memberId)
    {
        return [];
    }

    public function getDetails($memberId)
    {
        return [];
    }

    public function getLedger($memberId)
    {
        return [];
    }

    public function getLog($memberId)
    {
        return [];
    }

    public function getInfo($memberId)
    {
        return [];
    }

    /////////////
    // Actions //
    /////////////

    public function create($parentId, $data)
    {
        // Validate $data
        // Save to Database
        return new Result(true);
    }

    public function edit($memberId, $data)
    {
        // Validate $data
        // Update Database
        return new Result(true);
    }

    public function ban($memberId)
    {
        // Update Database
        return new Result(true);
    }

    public function unban($memberId)
    {
        // Update Database
        return new Result(true);
    }

    public function delete($memberId)
    {
        // Update Database
        return new Result(true);
    }

    public function topUp($memberId, $amount)
    {
        // Validate $amount
        // Update Database
        return new Result(true);
    }

    public function withdraw($memberId, $amount)
    {
        // Validate $amount
        // Update Database
        return new Result(true);
    }

    public function resetPasswordAndPin($memberId, $data)
    {
        // Validate $data
        // Update Database
        return new Result(true);
    }

}

In MemberController.php, I do a lot of tedious copy-and-paste, so I think it violates the DRY principle.

I think that, by using this approach, what I basically do is just moving all sequential tasks (check permission, validate input, save to database) from Controller to Repository. I think that my MemberRepository.php class is still ugly.

I created a MemberValidator class to help shorten the MemberRepository, but I think it only helps a little.

I will have other entities, like Post, Transaction, Report. I am not sure if I could use this approach again for them.

Is there any way to make my code shorter and cleaner?

Answer 1

I am not an expert, but what I am doing is to try to maintain Single Class Responsibility in my project.

Basically organize your project like this:

1. Create controllers as you wish (without logic) (no business logic here)
2. Create eloquent models as your database schema is (no business logic here)
3. Create repositories for all models
4. Create small services that are responsible only for one thing

Now we have 4 different beings in project: controller, model, repository, service. Controller's responsibility is to translate business logic call from user and to user. All it has to process request, notify proper service "we are doing something", get result and return view/json/whatever to client. There is NO PLACE AT ALL for business logic in controllers.

Model's responsibility is to translate DB table row into nice, elegant syntax. It's just like the definition of database table row. Here you will insert table names, whether it is soft deleting, translatable, has timestamps, and what relationship it has. It's just raw information how that model is handled without business logic.

Repository's responsibility is to create an "API" to manage your models, instead of managing them directly. Here you will want to have getUserById, getUserWithOrders, getInvoicesForUser type functions.

Now the "fourth element" you are looking for is a generic service. It's a simple class registered in IOC container. You should have many services and every service should implement a part of business logic system. So you may have AuthService, OrderService, and InvoiceService.

Whenever your user want to login, you do not login him with Auth::attempt in controller. Instead, invoke your AuthService from container, like App::Make('AuthService') and then call ->login($input) from here. The authorization logic should belong in the AuthService login method, even if it's simply a one-liner Auth::attempt. AuthService could also handle registering an user, so add an AuthService->register($input) method! In this method, you will talk to the user repository to create an user from input. It's ok for one service (Auth) to use another (UserRepository).

Now let's say you would want also to send notification email after user registers, right? After creating user you will add a new line to send email from AuthService... STOP! Now ask yourself: is the AUTHservice responsible of sending emails or not? No it is not. Instead, create another service, EmailNotificationService. In AuthService after registering the user, make Event::fire('AuthService.onUserRegistered' , $user) an event. Your AuthService now only registers users and notifies the whole system: "HEY I AM JUST REGISTERED SOMEONE!". Add Event::listen('AuthService.onUserRegistered') somewhere in your project and make it create an EmailNotificationService and invoke their sendRegistrationEmail method.

That how should it work for me. Just remember 3 things:

1. Do not put business logic in controller - they are only a "plugs" that your user connects to your app.
2. Use dependency injection.
3. Always remember about single class responsibility. If it's OrderService, let it only manage orders, not sending email notifications, generating invoices and such. Create a net of small-medium sized services that are loosely coupled with dependency injection and the Laravel event system.

An example from my system (logging into the system):

AuthController method:

public function loginWithEmail() {

    $authService = App::make( 'Auth.Email' );
    $handler = FormHandler::make( $authService->auth() );
    $handler->withView( 'site.auth.login_email' );
    $handler->success( Redirect::to( URL::action( 'AccountController@informations' ) ) );
    return $handler->exec();

}

AuthEmailService method:

public function auth() {

    $loginForm = new LoginForm();
    $loginForm->onSubmit( function( $data ) {

        $shouldRemember = ( isset( $data['remember'] ) && $data['remember'] == 1 ) ? true : false;

        $userEmail = $data['email'];
        $userPassword = $data['password'];

        if ( Auth::attempt( [ 'email' => $userEmail , 'password' => $userPassword , 'type' => UserType::EMAIL ] , $shouldRemember ) ) {

            Event::fire( 'Auth.LoggedIn' , Auth::user() );
            return true;

        } else {

            return [ 'general' => trans( 'auth.errors.invalid-credentials' ) ];

        }

    } );

    return $loginForm;

}

This is my simple decoupled system. FormHandler and FormObject are my private extensions to keep the code simple and organized. If you want, I can share these modules with you, but I have no documentation for them as they are for my private use.