@zachleigh, I see that makes sense, but isn't repositories, a good thing to have in case of app scaling? I'm not saying my CRM is gonna be the new Salesforce or anything, but rather than having everything in the controller like I did?, it seems like it gives way more flexibility. I see how the Interface might have been a bit overkill.

There are many other ways to scale. Check out this series: https://laracasts.com/series/whip-monstrous-code-into-shape

And I dont know, maybe this is just me, but before taking all the time to get my app ready to scale, I'd like to first validate my idea and see if Im going to need to scale it at all. The vast majority of apps wont need much work to scale to their use level.

In not-so-big applications I often put most of the logic in the models or if it's a lot of code I could use repository or if it's something to be shared between different models then I used traits.

Here's example code from my User model..

    /*
    |--------------------------------------------------------------------------
    | Methods
    |--------------------------------------------------------------------------
    */

    /**
     * Generate a new password and email it to user
     *
     * @return bool
     */
    public function emailNewPassword()
    {
        $password = str_random(8);
        $user = $this;
        $user->updatePassword($password);

        \Mail::send('common.email.new_password', ['user' => $user, 'password' => $password], function ($mail) use ($user) {
            $mail->to($user->email, $user->name)->subject('New password');
        });

        return true;
    }

    /**
     * Update password
     *
     * @param  string $password
     *
     * @return bool
     */
    public function updatePassword($password)
    {
        $this->password = bcrypt($password);
        $this->save();

        return true;
    }

I understand some of the points that @zachleigh has made and they're valid, but each to their own I guess! On a more positive note, the series that he recommended is pretty darn good!

Personally, I like the repository/interface pattern and I think its a sensible move to make early on whilst your still able to keep track of all the queries your making. You might never change your ORM from Eloquent, so the interfaces might never be used but you have that option now.

You could improve them further if you wanted by extending an abstract class and removing some of the base queries such as create/delete to there.

Department Repository

<?php
namespace App\Repositories\Department;

use App\Department;
use App\Repositories\BaseRepository;

class DepartmentRepository extends BaseRepository implements DepartmentRepositoryContract
 {
    protected $model;

    public function __construct() 
    {
        $this->model = new Department;
    }

    public function getAllDepartments()
    {
        return $this->model->all();
    }

    public function listAllDepartments()
    {
        return $this->model->lists('name', 'id');
    }
}

BaseRepository

<?php

namespace App\Repositories;

abstract class BaseRepository 
{
    public function create($input)
    {
        return $this->model->create($input);
    }

    public function delete($id)
    {
        return $this->model->destroy($id);
    }
}

Something like this should work.

I'd maybe change the name of your ServiceProvider too, I was automatically looking for a RepositoriesServiceProvider and it's called AccessServiceProvider. You might have a reason for that which I've just not considered though so feel free to tell me to shut up!

p.s. it's much easier writing code in an editor than in here, I wont be making that mistake again!

@andrewclark, I'll try to give abstract classes a look as well. It's funny to see how people are doing things differently. @zachleigh Thank you for the recommendation, I will give it a look, what you are saying with validating if it needs to scale etc, makes a lot of sense, but as this is mostly a learning project for me, I am trying out different approaches, and not so worried if it really needs to be able to scale atm.. @SaeedPrez Thanks for the input, I use my models mostly for simple eloquent(hasMany, belongsTo etc) commands, Dunno, if it just better when it's a small application to insert the logic into the models?

Thanks for all the input! Nice to hear the different approaches people would be taking.

@cawex as long as you don't put your logic in your routes, controllers or views... You got plenty of ideas here, decide what makes sense to you and fits your project and your needs.

UPDATE Nevermind, i figured it out :)

@ZetecVan Okay I didn't totally figure it out, the current problem is that I have copied for example the part where you can see the client info and user info(the header) into each view, and I get the data like this in the tasks view fx:

$client->userAssignee->name
$task->assigned->name

As I have to get the name of the user who is assigned the client or task, or the client assigned the task etc. I guess I can't really do this with a view composer, as the data is fairly dynamic, I would have to have a view composer for each blade template? or how would you go about it?

@cawex I'm not totally following. Can you tell me the route you are talking about, so I can have a look at the existing code on github.

@ZetecVan ofc, it's:

Tasks.show (https://github.com/Bottelet/Flarepoint-crm/blob/master/resources/views/tasks/show.blade.php#L20) Clients.show (https://github.com/Bottelet/Flarepoint-crm/blob/master/resources/views/clients/show.blade.php#L39) User.show (https://github.com/Bottelet/Flarepoint-crm/blob/master/resources/views/users/show.blade.php#L22)

as you can see they all have the same header but, the data is retrieved differently, as in tasks, for example, it has to get the User and Client that are assigned the task, so it goes through the task model, to get the associated user and client. Does it make sense ?

Edit: These 3 are the exact same data we are getting but retrieved in a different way https://github.com/Bottelet/Flarepoint-crm/blob/master/resources/views/users/show.blade.php#L27 (Here it just show's the users information on users.show)

https://github.com/Bottelet/Flarepoint-crm/blob/master/resources/views/tasks/show.blade.php#L86 (Here it shows the user who is assigned to the task)

https://github.com/Bottelet/Flarepoint-crm/blob/master/resources/views/clients/show.blade.php#L98 (Here it shows the user who is assigned to the client)

But it's the exact same view, just different approaches to getting the data. Make sense? :)

@cawex I have no insight to offer, just wanted to say nice job. As someone just getting into Laravel i'm enjoying looking through this

@cawex remember one thing a lot of the folks that try to push a repository on you some of them probably are just trying to sound good and fancy. Let's say you had 10000 lines of controller code and you decided to keep it there verses putting a large chunk of it in Repository.

Well I've got news for everyone the CPU does not care where the code is coming from, heck it could be coming from a file called Gonzalez or Billy Bob, it's going to process it the same no matter where it comes from and that my friend is an Undisputed fact, can't even argue it. So if you feel comfortable putting code in a controller that's fine.

Don't let people push patterns on you most of them are totally unnecessary when it comes to processing.

Well that doesn't say to not have special classes so you don't repeat yourself I'm not referring to that.

Bottom line whether you have a fat controller or Repository it ends up being the exact same thing just code is at different places.

@SaeedPrez aut oh I bet I opened another kettle of worms.

@jlrdw ☺

I agree with you, the CPU doesn't care and he should do what he's comfortable with.

With that being said, being a site like Laracasts where I assume a lot of people (including a lot of beginners) read these posts, I try to recommend better practice more often than not. My thought is everybody probably start by putting most of their code in their routes, views and controllers (I did when I started) which has the potential to cause issues, so maybe we can teach people to try a different way and then they can decide for themselves.

@cawex Ah I see now. What I'd be tempted to do is make one partial view called contacts.blade.php and have

    <!--Client info leftside-->
    <div class="contactleft">
      @if($contact->email != "")
      <!--MAIL-->
      <p> <span class="glyphicon glyphicon-envelope" aria-hidden="true" data-toggle="tooltip" title="Email" data-placement="left" > </span>
      <a href="mailto:{{$contact->email}}" data-toggle="tooltip" data-placement="left">{{$contact->email}}</a></p>
      @endif
      @if($contact->primary_number != "")
      <!--Work Phone-->
      <p> <span class="glyphicon glyphicon-headphones" aria-hidden="true" data-toggle="tooltip" title="Primary number" data-placement="left"> </span>
      <a href="tel:{{$contact->work_number}}">{{$contact->primary_number}}</a></p>
      @endif
      @if($contact->secondary_number != "")
      <!--Secondary Phone-->
      <p> <span class="glyphicon glyphicon-phone" aria-hidden="true" data-toggle="tooltip" title="Secondary number" data-placement="left"> </span>
      <a href="tel:{{$contact->secondary_number}}">{{$contact->secondary_number}}</a></p>
      @endif
      @if($contact->address || $contact->zipcode || $contact->city != "")
      <!--Address-->
      <p> <span class="glyphicon glyphicon-home" aria-hidden="true" data-toggle="tooltip" title="Address/Zip code/city" data-placement="left"> </span>  {{$contact->address}} <br />{{$contact->zipcode}} {{$contact->city}}
    </p>
    @endif
  </div>

I've changed the field names so its $contact->.This way, you've only got one place to change if things need to change. Then I'd have the boot method in my composer service provider set like

    public function boot()
    {
        View::composer('clients.show', 'App\Http\ViewComposers\ClientsDashboardComposer');
        View::composer('tasks.show', 'App\Http\ViewComposers\TasksDashboardComposer');
        View::composer('users.show', 'App\Http\ViewComposers\UsersDashboardComposer');
    }

so it's triggering on the full view rather than the partial.

Then in each Composer, I'd set $contact differently. So in ClientsDashboardComposer I'd have $contact = $client, in UsersDashboardComposer I'd have $contact = $user, and in TasksDashboardComposer I'd have $contact = $tasks->clientAssignee. eg;

// Task View
$tasks = $this->tasks->all(); // or however you get them
$contact = $tasks->clientAssignee;

This will only work if there's consistency over the model field names.

Does that help?

Food for thought with a repository pattern:

If you utilize eloquent many times your repo with just wrap an eloquent call. Eloquent, for a lack of better terminology atm, is a wrapper already to build a query from a model. Which is arguably very similar to a basic repo.

Example: Function create($object) { model->create($object) }

Secondly interfaces tend to be a waste of space. Unless you're implementing like a email from mailgun but you plan on setting up your own email server later. Seems a waste of server resources loaded all the bindings into memory every request. If you swap from MySQL to mongo, a few classes are the least of your worries.

Example I see a lot: Quick update, add another method in a repo, never update the interface. 10 quick updates later interface only has half the methods. Point of interface nullified.

Because of eloquent and trying to keep files and amount of files lean I tend to utilize traits on models or just the model it's self. Use eloquent or readable query scopes in the controller. For large queries from multiple tables like your dashboard I may create a method that returns a collection (one variable) using laravels collection classes.

But yes, each person and app is different. At the end of the day, better is an opinion and all that matters is that it works.

Interfaces tend to add information on how to properly interface with objects and define types. Its a commonly accepted practice.

@jlrdw Thank you, that actually kind of relieved me, even though I knew it, hearing it just made it more "sensible" :)

@jaydeluca Thank you very much, I appreciate it.

@ZetecVan Thanks, I will look into it later, see if I can get it work, it make sense they way you tell it, let's hope I can do it in practice.

@jekinney Makes a lot of sense, but I guess it's also a lot about just being strict and stick to the pattern you chose then. With the dashboard, where would you create the method that returns a collection? Which approach would you take to do it? I don't totally follow there I have to admit, any examples of some sort?

  public function create()
    {
        return view('departments.create');
    }
    public function store(StoreDepartmentRequest $request)
    {
        $this->departments->create($request);
        Session::flash('flash_message', 'Successfully created New Department');
        return redirect()->route('departments.index');
    }
    public function destroy($id)
    {
        $this->departments->destroy($id);
        return redirect()->route('departments.index');

I think you forgot to add comments. ;)

https://leanpub.com/laravelcollectionsunraveled

One of many.

@jimmck true, but in reality it doesn't happen. At least the code I've come across and to be honest myself when a client calls needed some quick update (and always needed yesterday).

When it comes to using repositories or fat controllers technically there is no best practice either way is perfectly acceptable, I originally came from Java technologies and I had some pretty fat servlets but heck they did the job. And not to boast but I make damn good money doing what I did.

So as a couple other users stated at the end of the day the main thing is: Is the code secure foremost, and does it do the job it was meant to do. That said I am not arguing with anyone just stating what I believe is true.

Edit: I particularly don't like patterns just regular MVC just like in documentation.

@ZetecVan I've got it to work! I thank you for the help, it is pushed and up and running :)

@sadhakbj Do you mean doc blocks, or the session flash message? :) Well otherwise both are missing, the flash message is an error, and more comment/doc blocks are on the to-do, but thank you!

@Cawex Excellent. I'll take a look :-)