Laravel UserController Design

The name of the pictureThe name of the pictureThe name of the pictureClash Royale CLAN TAG#URR8PPP





.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty margin-bottom:0;







up vote
4
down vote

favorite
1












<?php

namespace AppHttpControllers;

use IlluminateHttpRequest;
use AppUser;
use Auth;

class ProfileController extends Controller


public function __construct()


$this->middleware('auth');



public function index($id)


$user = User::find($id);

$photos = $user->getPhotos();

$interests = $user->getInterests();

$gifts = $user->getGifts();

$visits = $user->totalVisits();

$likes = $user->totalLikes();

if ($user->id == Auth::user()->id)

$page_name = 'My Profile';

return view('profile.my_profile', compact('page_name', 'user', 'photos', 'interests', 'likes', 'visits', 'gifts'));

else

$page_name = $user->name;

return view('profile.profile', compact('page_name', 'user', 'photos', 'interests', 'likes', 'visits', 'gifts'));








I have the following controller, which manages the "Profile" page in my web app. I feel like in my current code, there is a separation in a way that photos, interests, gifts, visits and likes look like they do not belong to the User.



How would I go to refactor all these different functions into one? Should I change the User Model's __construct to execute all of the functions? Ideally, I would like to pass only two variables to my view ("user" and "page_name" variables)



Here is my model, for reference:



<?php

namespace App;

use IlluminateFoundationAuthUser as Authenticatable;
use IlluminateNotificationsNotifiable;
use IlluminateSupportFacadesDB;

class User extends Authenticatable

use Notifiable;

protected $table = 'users';

public $timestamps = false;

/**
* The attributes that are mass assignable.
*
* @var array
*/

protected $fillable = [
'name', 'email', 'password',
];

/**
* The attributes that should be hidden for arrays.
*
* @var array
*/

protected $hidden = [
'password', 'remember_token',
];

public function getPhotos()


return DB::table('photos')
->where('user_id', $this->id)
->get();



public function getInterests()


return DB::table('interests')
->where('user_id', $this->id)
->get();



public function getGifts()


return DB::table('gifts')
->join('sent_gifts', function ($join)
$join->on('gifts.id', '=', 'sent_gifts.gift_id')
->where('user_2', $this->id);
)
->join('users', function ($join)
$join->on('users.id', '=', 'sent_gifts.user_1');
)
->get();



public function getVisits()


return DB::table('user_reactions')
->join('users', function ($join)
$join->on('users.id', '=', 'user_reactions.user_1')
->where('user_2', $this->id)
->where('reaction', 'visit');
)
->get();



public function getLikes()


return DB::table('user_reactions')
->join('users', function ($join)
$join->on('users.id', '=', 'user_reactions.user_1')
->where('user_2', $this->id)
->where('reaction', 'like');
)
->get();



public function getFilter()


return DB::table('filters')
->where('user_id', $this->id)
->limit(1)
->get();



public function totalVisits()


return DB::table('user_reactions')
->where('reaction', 'visit')
->where('user_2', $this->id)
->get()
->count();



public function totalLikes()


return DB::table('user_reactions')
->where('reaction', 'like')
->where('user_2', $this->id)
->get()
->count();










share|improve this question















  • 2




    It looks like you're not using Laravel relationships which reduce a lot of your code in the User model. See here laravel.com/docs/5.6/eloquent-relationships
    – Josh Griggs
    May 14 at 12:28










  • What version of Laravel are you using?
    – Ross Wilson
    May 30 at 7:39










  • @RossWilson Laravel 5.4
    – garjoka madafa
    May 30 at 17:44

















up vote
4
down vote

favorite
1












<?php

namespace AppHttpControllers;

use IlluminateHttpRequest;
use AppUser;
use Auth;

class ProfileController extends Controller


public function __construct()


$this->middleware('auth');



public function index($id)


$user = User::find($id);

$photos = $user->getPhotos();

$interests = $user->getInterests();

$gifts = $user->getGifts();

$visits = $user->totalVisits();

$likes = $user->totalLikes();

if ($user->id == Auth::user()->id)

$page_name = 'My Profile';

return view('profile.my_profile', compact('page_name', 'user', 'photos', 'interests', 'likes', 'visits', 'gifts'));

else

$page_name = $user->name;

return view('profile.profile', compact('page_name', 'user', 'photos', 'interests', 'likes', 'visits', 'gifts'));








I have the following controller, which manages the "Profile" page in my web app. I feel like in my current code, there is a separation in a way that photos, interests, gifts, visits and likes look like they do not belong to the User.



How would I go to refactor all these different functions into one? Should I change the User Model's __construct to execute all of the functions? Ideally, I would like to pass only two variables to my view ("user" and "page_name" variables)



Here is my model, for reference:



<?php

namespace App;

use IlluminateFoundationAuthUser as Authenticatable;
use IlluminateNotificationsNotifiable;
use IlluminateSupportFacadesDB;

class User extends Authenticatable

use Notifiable;

protected $table = 'users';

public $timestamps = false;

/**
* The attributes that are mass assignable.
*
* @var array
*/

protected $fillable = [
'name', 'email', 'password',
];

/**
* The attributes that should be hidden for arrays.
*
* @var array
*/

protected $hidden = [
'password', 'remember_token',
];

public function getPhotos()


return DB::table('photos')
->where('user_id', $this->id)
->get();



public function getInterests()


return DB::table('interests')
->where('user_id', $this->id)
->get();



public function getGifts()


return DB::table('gifts')
->join('sent_gifts', function ($join)
$join->on('gifts.id', '=', 'sent_gifts.gift_id')
->where('user_2', $this->id);
)
->join('users', function ($join)
$join->on('users.id', '=', 'sent_gifts.user_1');
)
->get();



public function getVisits()


return DB::table('user_reactions')
->join('users', function ($join)
$join->on('users.id', '=', 'user_reactions.user_1')
->where('user_2', $this->id)
->where('reaction', 'visit');
)
->get();



public function getLikes()


return DB::table('user_reactions')
->join('users', function ($join)
$join->on('users.id', '=', 'user_reactions.user_1')
->where('user_2', $this->id)
->where('reaction', 'like');
)
->get();



public function getFilter()


return DB::table('filters')
->where('user_id', $this->id)
->limit(1)
->get();



public function totalVisits()


return DB::table('user_reactions')
->where('reaction', 'visit')
->where('user_2', $this->id)
->get()
->count();



public function totalLikes()


return DB::table('user_reactions')
->where('reaction', 'like')
->where('user_2', $this->id)
->get()
->count();










share|improve this question















  • 2




    It looks like you're not using Laravel relationships which reduce a lot of your code in the User model. See here laravel.com/docs/5.6/eloquent-relationships
    – Josh Griggs
    May 14 at 12:28










  • What version of Laravel are you using?
    – Ross Wilson
    May 30 at 7:39










  • @RossWilson Laravel 5.4
    – garjoka madafa
    May 30 at 17:44













up vote
4
down vote

favorite
1









up vote
4
down vote

favorite
1






1





<?php

namespace AppHttpControllers;

use IlluminateHttpRequest;
use AppUser;
use Auth;

class ProfileController extends Controller


public function __construct()


$this->middleware('auth');



public function index($id)


$user = User::find($id);

$photos = $user->getPhotos();

$interests = $user->getInterests();

$gifts = $user->getGifts();

$visits = $user->totalVisits();

$likes = $user->totalLikes();

if ($user->id == Auth::user()->id)

$page_name = 'My Profile';

return view('profile.my_profile', compact('page_name', 'user', 'photos', 'interests', 'likes', 'visits', 'gifts'));

else

$page_name = $user->name;

return view('profile.profile', compact('page_name', 'user', 'photos', 'interests', 'likes', 'visits', 'gifts'));








I have the following controller, which manages the "Profile" page in my web app. I feel like in my current code, there is a separation in a way that photos, interests, gifts, visits and likes look like they do not belong to the User.



How would I go to refactor all these different functions into one? Should I change the User Model's __construct to execute all of the functions? Ideally, I would like to pass only two variables to my view ("user" and "page_name" variables)



Here is my model, for reference:



<?php

namespace App;

use IlluminateFoundationAuthUser as Authenticatable;
use IlluminateNotificationsNotifiable;
use IlluminateSupportFacadesDB;

class User extends Authenticatable

use Notifiable;

protected $table = 'users';

public $timestamps = false;

/**
* The attributes that are mass assignable.
*
* @var array
*/

protected $fillable = [
'name', 'email', 'password',
];

/**
* The attributes that should be hidden for arrays.
*
* @var array
*/

protected $hidden = [
'password', 'remember_token',
];

public function getPhotos()


return DB::table('photos')
->where('user_id', $this->id)
->get();



public function getInterests()


return DB::table('interests')
->where('user_id', $this->id)
->get();



public function getGifts()


return DB::table('gifts')
->join('sent_gifts', function ($join)
$join->on('gifts.id', '=', 'sent_gifts.gift_id')
->where('user_2', $this->id);
)
->join('users', function ($join)
$join->on('users.id', '=', 'sent_gifts.user_1');
)
->get();



public function getVisits()


return DB::table('user_reactions')
->join('users', function ($join)
$join->on('users.id', '=', 'user_reactions.user_1')
->where('user_2', $this->id)
->where('reaction', 'visit');
)
->get();



public function getLikes()


return DB::table('user_reactions')
->join('users', function ($join)
$join->on('users.id', '=', 'user_reactions.user_1')
->where('user_2', $this->id)
->where('reaction', 'like');
)
->get();



public function getFilter()


return DB::table('filters')
->where('user_id', $this->id)
->limit(1)
->get();



public function totalVisits()


return DB::table('user_reactions')
->where('reaction', 'visit')
->where('user_2', $this->id)
->get()
->count();



public function totalLikes()


return DB::table('user_reactions')
->where('reaction', 'like')
->where('user_2', $this->id)
->get()
->count();










share|improve this question











<?php

namespace AppHttpControllers;

use IlluminateHttpRequest;
use AppUser;
use Auth;

class ProfileController extends Controller


public function __construct()


$this->middleware('auth');



public function index($id)


$user = User::find($id);

$photos = $user->getPhotos();

$interests = $user->getInterests();

$gifts = $user->getGifts();

$visits = $user->totalVisits();

$likes = $user->totalLikes();

if ($user->id == Auth::user()->id)

$page_name = 'My Profile';

return view('profile.my_profile', compact('page_name', 'user', 'photos', 'interests', 'likes', 'visits', 'gifts'));

else

$page_name = $user->name;

return view('profile.profile', compact('page_name', 'user', 'photos', 'interests', 'likes', 'visits', 'gifts'));








I have the following controller, which manages the "Profile" page in my web app. I feel like in my current code, there is a separation in a way that photos, interests, gifts, visits and likes look like they do not belong to the User.



How would I go to refactor all these different functions into one? Should I change the User Model's __construct to execute all of the functions? Ideally, I would like to pass only two variables to my view ("user" and "page_name" variables)



Here is my model, for reference:



<?php

namespace App;

use IlluminateFoundationAuthUser as Authenticatable;
use IlluminateNotificationsNotifiable;
use IlluminateSupportFacadesDB;

class User extends Authenticatable

use Notifiable;

protected $table = 'users';

public $timestamps = false;

/**
* The attributes that are mass assignable.
*
* @var array
*/

protected $fillable = [
'name', 'email', 'password',
];

/**
* The attributes that should be hidden for arrays.
*
* @var array
*/

protected $hidden = [
'password', 'remember_token',
];

public function getPhotos()


return DB::table('photos')
->where('user_id', $this->id)
->get();



public function getInterests()


return DB::table('interests')
->where('user_id', $this->id)
->get();



public function getGifts()


return DB::table('gifts')
->join('sent_gifts', function ($join)
$join->on('gifts.id', '=', 'sent_gifts.gift_id')
->where('user_2', $this->id);
)
->join('users', function ($join)
$join->on('users.id', '=', 'sent_gifts.user_1');
)
->get();



public function getVisits()


return DB::table('user_reactions')
->join('users', function ($join)
$join->on('users.id', '=', 'user_reactions.user_1')
->where('user_2', $this->id)
->where('reaction', 'visit');
)
->get();



public function getLikes()


return DB::table('user_reactions')
->join('users', function ($join)
$join->on('users.id', '=', 'user_reactions.user_1')
->where('user_2', $this->id)
->where('reaction', 'like');
)
->get();



public function getFilter()


return DB::table('filters')
->where('user_id', $this->id)
->limit(1)
->get();



public function totalVisits()


return DB::table('user_reactions')
->where('reaction', 'visit')
->where('user_2', $this->id)
->get()
->count();



public function totalLikes()


return DB::table('user_reactions')
->where('reaction', 'like')
->where('user_2', $this->id)
->get()
->count();












share|improve this question










share|improve this question




share|improve this question









asked May 14 at 5:45









garjoka madafa

233




233







  • 2




    It looks like you're not using Laravel relationships which reduce a lot of your code in the User model. See here laravel.com/docs/5.6/eloquent-relationships
    – Josh Griggs
    May 14 at 12:28










  • What version of Laravel are you using?
    – Ross Wilson
    May 30 at 7:39










  • @RossWilson Laravel 5.4
    – garjoka madafa
    May 30 at 17:44













  • 2




    It looks like you're not using Laravel relationships which reduce a lot of your code in the User model. See here laravel.com/docs/5.6/eloquent-relationships
    – Josh Griggs
    May 14 at 12:28










  • What version of Laravel are you using?
    – Ross Wilson
    May 30 at 7:39










  • @RossWilson Laravel 5.4
    – garjoka madafa
    May 30 at 17:44








2




2




It looks like you're not using Laravel relationships which reduce a lot of your code in the User model. See here laravel.com/docs/5.6/eloquent-relationships
– Josh Griggs
May 14 at 12:28




It looks like you're not using Laravel relationships which reduce a lot of your code in the User model. See here laravel.com/docs/5.6/eloquent-relationships
– Josh Griggs
May 14 at 12:28












What version of Laravel are you using?
– Ross Wilson
May 30 at 7:39




What version of Laravel are you using?
– Ross Wilson
May 30 at 7:39












@RossWilson Laravel 5.4
– garjoka madafa
May 30 at 17:44





@RossWilson Laravel 5.4
– garjoka madafa
May 30 at 17:44











1 Answer
1






active

oldest

votes

















up vote
4
down vote



accepted










First of all do not use Laravel 5.4 even if you've written some* code already, use latest 5.6, and try to always update.



*: You really need to rewrite the application if you have code like you presented everywhere, so just take your time and use L5.6. Idea is already there you just need to improve it - pure coding, no thinking.



Model(s)



Standards



You are sticking to the standard (I guess because User model is the stock one) and using class name User for a model means that table is plural which renders $table property useless and you save one line.



Each table shall have its own Model.




Note that we did not tell Eloquent which table to use for our Flight model. By convention, the "snake case", plural name of the class will be used as the table name unless another name is explicitly specified.





Relationships



Main reason you have read about Laravel when you were googling around which framework to use is because its ORM Eloquent*. Eloquent is a abstraction so you do not even need to know how each relationship work under the hood, it does all the magic for you. Firstly you'll need to understand what relationships are so google around, and when you are done use superb documentation of Laravel** and get all the info from there.



*,**: In early days of Laravel it was the selling point for many developers.



Defining the relationships is hard part of RDBMS design, so take your time, do it right, use pen and paper and then code it - easy part if design is right.




Note: For user_reactions use polymorphic relations.




Invest time to learning relationships it will pay off soon :)




Note: I am not writing about how I would define relations you have in code because of lack of context and mainly because of helping too much and I consider it as giving a fish instead of teaching how to fish.




Controller



Routing / Middleware / Route-Model-Binding



Forget about defining middlewares in controller itself, use routes file for that, so delete the __construct() and just add it back to the routes file. You may ask why, well because of DRY, you will end up with 5-x controllers and each of them will contain $this->middleware('auth'); in constructor, repeating...*



*: Unless you make a "master" class like AuthController and extend from there - IMO that just adds more complications than good if you want to define middleware only.



Use groups in route file, and define middleware on the groups part, see below for route example.




Route Model Binding another super useful feature of Laravel. Example vs bunch of words:



Route:



Route::middleware(['auth'])->group(function () 
Route::get('profile/user', 'ProfileController@index'); <-- Route Model Binding
Route::get('profile/edit', 'ProfileController@edit');
);


Signature of ProfileController@index method should be index(AppUser $user), you see behind the scenes Laravel does query to the database and fetches User instance for you.




After defining all the relationships on User model you will most likely end up with using a oneliner to get the $user:



$user = User::with(['photos', 'interests', 'gifts', 'visits', 'likes'])->whereId($id)->get(); // if you stick with not using Route Model Binding
$user->load(['photos', 'interests', 'gifts', 'visits', 'likes']); // if you use Route Model Binding

// in view you will access interests for example $user->interests


Now what about edge case if user is looking at their own profile, as I understand the only difference is to change variable $page_name, do it right in the view (I would do it that way):



@if($user->id == Auth::user()->id)
My Profile
@else
$user->name
@endif


So proper controller after suggestions would look like:



<?php
namespace AppHttpControllers;

use AppUser;
use Auth;

class ProfileController extends Controller


public function index(User $user)

$user->load(['photos', 'interests', 'gifts', 'visits', 'likes']);

return view('profile.my_profile', compact('user'));





Packages



Do not reinvent the wheel, for example for handling attachments / images use solution that is tested https://github.com/spatie/laravel-medialibrary and used by many!



During development debugbar is a must.






share|improve this answer























    Your Answer




    StackExchange.ifUsing("editor", function ()
    return StackExchange.using("mathjaxEditing", function ()
    StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix)
    StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
    );
    );
    , "mathjax-editing");

    StackExchange.ifUsing("editor", function ()
    StackExchange.using("externalEditor", function ()
    StackExchange.using("snippets", function ()
    StackExchange.snippets.init();
    );
    );
    , "code-snippets");

    StackExchange.ready(function()
    var channelOptions =
    tags: "".split(" "),
    id: "196"
    ;
    initTagRenderer("".split(" "), "".split(" "), channelOptions);

    StackExchange.using("externalEditor", function()
    // Have to fire editor after snippets, if snippets enabled
    if (StackExchange.settings.snippets.snippetsEnabled)
    StackExchange.using("snippets", function()
    createEditor();
    );

    else
    createEditor();

    );

    function createEditor()
    StackExchange.prepareEditor(
    heartbeatType: 'answer',
    convertImagesToLinks: false,
    noModals: false,
    showLowRepImageUploadWarning: true,
    reputationToPostImages: null,
    bindNavPrevention: true,
    postfix: "",
    onDemand: true,
    discardSelector: ".discard-answer"
    ,immediatelyShowMarkdownHelp:true
    );



    );








     

    draft saved


    draft discarded


















    StackExchange.ready(
    function ()
    StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f194343%2flaravel-usercontroller-design%23new-answer', 'question_page');

    );

    Post as a guest






























    1 Answer
    1






    active

    oldest

    votes








    1 Answer
    1






    active

    oldest

    votes









    active

    oldest

    votes






    active

    oldest

    votes








    up vote
    4
    down vote



    accepted










    First of all do not use Laravel 5.4 even if you've written some* code already, use latest 5.6, and try to always update.



    *: You really need to rewrite the application if you have code like you presented everywhere, so just take your time and use L5.6. Idea is already there you just need to improve it - pure coding, no thinking.



    Model(s)



    Standards



    You are sticking to the standard (I guess because User model is the stock one) and using class name User for a model means that table is plural which renders $table property useless and you save one line.



    Each table shall have its own Model.




    Note that we did not tell Eloquent which table to use for our Flight model. By convention, the "snake case", plural name of the class will be used as the table name unless another name is explicitly specified.





    Relationships



    Main reason you have read about Laravel when you were googling around which framework to use is because its ORM Eloquent*. Eloquent is a abstraction so you do not even need to know how each relationship work under the hood, it does all the magic for you. Firstly you'll need to understand what relationships are so google around, and when you are done use superb documentation of Laravel** and get all the info from there.



    *,**: In early days of Laravel it was the selling point for many developers.



    Defining the relationships is hard part of RDBMS design, so take your time, do it right, use pen and paper and then code it - easy part if design is right.




    Note: For user_reactions use polymorphic relations.




    Invest time to learning relationships it will pay off soon :)




    Note: I am not writing about how I would define relations you have in code because of lack of context and mainly because of helping too much and I consider it as giving a fish instead of teaching how to fish.




    Controller



    Routing / Middleware / Route-Model-Binding



    Forget about defining middlewares in controller itself, use routes file for that, so delete the __construct() and just add it back to the routes file. You may ask why, well because of DRY, you will end up with 5-x controllers and each of them will contain $this->middleware('auth'); in constructor, repeating...*



    *: Unless you make a "master" class like AuthController and extend from there - IMO that just adds more complications than good if you want to define middleware only.



    Use groups in route file, and define middleware on the groups part, see below for route example.




    Route Model Binding another super useful feature of Laravel. Example vs bunch of words:



    Route:



    Route::middleware(['auth'])->group(function () 
    Route::get('profile/user', 'ProfileController@index'); <-- Route Model Binding
    Route::get('profile/edit', 'ProfileController@edit');
    );


    Signature of ProfileController@index method should be index(AppUser $user), you see behind the scenes Laravel does query to the database and fetches User instance for you.




    After defining all the relationships on User model you will most likely end up with using a oneliner to get the $user:



    $user = User::with(['photos', 'interests', 'gifts', 'visits', 'likes'])->whereId($id)->get(); // if you stick with not using Route Model Binding
    $user->load(['photos', 'interests', 'gifts', 'visits', 'likes']); // if you use Route Model Binding

    // in view you will access interests for example $user->interests


    Now what about edge case if user is looking at their own profile, as I understand the only difference is to change variable $page_name, do it right in the view (I would do it that way):



    @if($user->id == Auth::user()->id)
    My Profile
    @else
    $user->name
    @endif


    So proper controller after suggestions would look like:



    <?php
    namespace AppHttpControllers;

    use AppUser;
    use Auth;

    class ProfileController extends Controller


    public function index(User $user)

    $user->load(['photos', 'interests', 'gifts', 'visits', 'likes']);

    return view('profile.my_profile', compact('user'));





    Packages



    Do not reinvent the wheel, for example for handling attachments / images use solution that is tested https://github.com/spatie/laravel-medialibrary and used by many!



    During development debugbar is a must.






    share|improve this answer



























      up vote
      4
      down vote



      accepted










      First of all do not use Laravel 5.4 even if you've written some* code already, use latest 5.6, and try to always update.



      *: You really need to rewrite the application if you have code like you presented everywhere, so just take your time and use L5.6. Idea is already there you just need to improve it - pure coding, no thinking.



      Model(s)



      Standards



      You are sticking to the standard (I guess because User model is the stock one) and using class name User for a model means that table is plural which renders $table property useless and you save one line.



      Each table shall have its own Model.




      Note that we did not tell Eloquent which table to use for our Flight model. By convention, the "snake case", plural name of the class will be used as the table name unless another name is explicitly specified.





      Relationships



      Main reason you have read about Laravel when you were googling around which framework to use is because its ORM Eloquent*. Eloquent is a abstraction so you do not even need to know how each relationship work under the hood, it does all the magic for you. Firstly you'll need to understand what relationships are so google around, and when you are done use superb documentation of Laravel** and get all the info from there.



      *,**: In early days of Laravel it was the selling point for many developers.



      Defining the relationships is hard part of RDBMS design, so take your time, do it right, use pen and paper and then code it - easy part if design is right.




      Note: For user_reactions use polymorphic relations.




      Invest time to learning relationships it will pay off soon :)




      Note: I am not writing about how I would define relations you have in code because of lack of context and mainly because of helping too much and I consider it as giving a fish instead of teaching how to fish.




      Controller



      Routing / Middleware / Route-Model-Binding



      Forget about defining middlewares in controller itself, use routes file for that, so delete the __construct() and just add it back to the routes file. You may ask why, well because of DRY, you will end up with 5-x controllers and each of them will contain $this->middleware('auth'); in constructor, repeating...*



      *: Unless you make a "master" class like AuthController and extend from there - IMO that just adds more complications than good if you want to define middleware only.



      Use groups in route file, and define middleware on the groups part, see below for route example.




      Route Model Binding another super useful feature of Laravel. Example vs bunch of words:



      Route:



      Route::middleware(['auth'])->group(function () 
      Route::get('profile/user', 'ProfileController@index'); <-- Route Model Binding
      Route::get('profile/edit', 'ProfileController@edit');
      );


      Signature of ProfileController@index method should be index(AppUser $user), you see behind the scenes Laravel does query to the database and fetches User instance for you.




      After defining all the relationships on User model you will most likely end up with using a oneliner to get the $user:



      $user = User::with(['photos', 'interests', 'gifts', 'visits', 'likes'])->whereId($id)->get(); // if you stick with not using Route Model Binding
      $user->load(['photos', 'interests', 'gifts', 'visits', 'likes']); // if you use Route Model Binding

      // in view you will access interests for example $user->interests


      Now what about edge case if user is looking at their own profile, as I understand the only difference is to change variable $page_name, do it right in the view (I would do it that way):



      @if($user->id == Auth::user()->id)
      My Profile
      @else
      $user->name
      @endif


      So proper controller after suggestions would look like:



      <?php
      namespace AppHttpControllers;

      use AppUser;
      use Auth;

      class ProfileController extends Controller


      public function index(User $user)

      $user->load(['photos', 'interests', 'gifts', 'visits', 'likes']);

      return view('profile.my_profile', compact('user'));





      Packages



      Do not reinvent the wheel, for example for handling attachments / images use solution that is tested https://github.com/spatie/laravel-medialibrary and used by many!



      During development debugbar is a must.






      share|improve this answer

























        up vote
        4
        down vote



        accepted







        up vote
        4
        down vote



        accepted






        First of all do not use Laravel 5.4 even if you've written some* code already, use latest 5.6, and try to always update.



        *: You really need to rewrite the application if you have code like you presented everywhere, so just take your time and use L5.6. Idea is already there you just need to improve it - pure coding, no thinking.



        Model(s)



        Standards



        You are sticking to the standard (I guess because User model is the stock one) and using class name User for a model means that table is plural which renders $table property useless and you save one line.



        Each table shall have its own Model.




        Note that we did not tell Eloquent which table to use for our Flight model. By convention, the "snake case", plural name of the class will be used as the table name unless another name is explicitly specified.





        Relationships



        Main reason you have read about Laravel when you were googling around which framework to use is because its ORM Eloquent*. Eloquent is a abstraction so you do not even need to know how each relationship work under the hood, it does all the magic for you. Firstly you'll need to understand what relationships are so google around, and when you are done use superb documentation of Laravel** and get all the info from there.



        *,**: In early days of Laravel it was the selling point for many developers.



        Defining the relationships is hard part of RDBMS design, so take your time, do it right, use pen and paper and then code it - easy part if design is right.




        Note: For user_reactions use polymorphic relations.




        Invest time to learning relationships it will pay off soon :)




        Note: I am not writing about how I would define relations you have in code because of lack of context and mainly because of helping too much and I consider it as giving a fish instead of teaching how to fish.




        Controller



        Routing / Middleware / Route-Model-Binding



        Forget about defining middlewares in controller itself, use routes file for that, so delete the __construct() and just add it back to the routes file. You may ask why, well because of DRY, you will end up with 5-x controllers and each of them will contain $this->middleware('auth'); in constructor, repeating...*



        *: Unless you make a "master" class like AuthController and extend from there - IMO that just adds more complications than good if you want to define middleware only.



        Use groups in route file, and define middleware on the groups part, see below for route example.




        Route Model Binding another super useful feature of Laravel. Example vs bunch of words:



        Route:



        Route::middleware(['auth'])->group(function () 
        Route::get('profile/user', 'ProfileController@index'); <-- Route Model Binding
        Route::get('profile/edit', 'ProfileController@edit');
        );


        Signature of ProfileController@index method should be index(AppUser $user), you see behind the scenes Laravel does query to the database and fetches User instance for you.




        After defining all the relationships on User model you will most likely end up with using a oneliner to get the $user:



        $user = User::with(['photos', 'interests', 'gifts', 'visits', 'likes'])->whereId($id)->get(); // if you stick with not using Route Model Binding
        $user->load(['photos', 'interests', 'gifts', 'visits', 'likes']); // if you use Route Model Binding

        // in view you will access interests for example $user->interests


        Now what about edge case if user is looking at their own profile, as I understand the only difference is to change variable $page_name, do it right in the view (I would do it that way):



        @if($user->id == Auth::user()->id)
        My Profile
        @else
        $user->name
        @endif


        So proper controller after suggestions would look like:



        <?php
        namespace AppHttpControllers;

        use AppUser;
        use Auth;

        class ProfileController extends Controller


        public function index(User $user)

        $user->load(['photos', 'interests', 'gifts', 'visits', 'likes']);

        return view('profile.my_profile', compact('user'));





        Packages



        Do not reinvent the wheel, for example for handling attachments / images use solution that is tested https://github.com/spatie/laravel-medialibrary and used by many!



        During development debugbar is a must.






        share|improve this answer















        First of all do not use Laravel 5.4 even if you've written some* code already, use latest 5.6, and try to always update.



        *: You really need to rewrite the application if you have code like you presented everywhere, so just take your time and use L5.6. Idea is already there you just need to improve it - pure coding, no thinking.



        Model(s)



        Standards



        You are sticking to the standard (I guess because User model is the stock one) and using class name User for a model means that table is plural which renders $table property useless and you save one line.



        Each table shall have its own Model.




        Note that we did not tell Eloquent which table to use for our Flight model. By convention, the "snake case", plural name of the class will be used as the table name unless another name is explicitly specified.





        Relationships



        Main reason you have read about Laravel when you were googling around which framework to use is because its ORM Eloquent*. Eloquent is a abstraction so you do not even need to know how each relationship work under the hood, it does all the magic for you. Firstly you'll need to understand what relationships are so google around, and when you are done use superb documentation of Laravel** and get all the info from there.



        *,**: In early days of Laravel it was the selling point for many developers.



        Defining the relationships is hard part of RDBMS design, so take your time, do it right, use pen and paper and then code it - easy part if design is right.




        Note: For user_reactions use polymorphic relations.




        Invest time to learning relationships it will pay off soon :)




        Note: I am not writing about how I would define relations you have in code because of lack of context and mainly because of helping too much and I consider it as giving a fish instead of teaching how to fish.




        Controller



        Routing / Middleware / Route-Model-Binding



        Forget about defining middlewares in controller itself, use routes file for that, so delete the __construct() and just add it back to the routes file. You may ask why, well because of DRY, you will end up with 5-x controllers and each of them will contain $this->middleware('auth'); in constructor, repeating...*



        *: Unless you make a "master" class like AuthController and extend from there - IMO that just adds more complications than good if you want to define middleware only.



        Use groups in route file, and define middleware on the groups part, see below for route example.




        Route Model Binding another super useful feature of Laravel. Example vs bunch of words:



        Route:



        Route::middleware(['auth'])->group(function () 
        Route::get('profile/user', 'ProfileController@index'); <-- Route Model Binding
        Route::get('profile/edit', 'ProfileController@edit');
        );


        Signature of ProfileController@index method should be index(AppUser $user), you see behind the scenes Laravel does query to the database and fetches User instance for you.




        After defining all the relationships on User model you will most likely end up with using a oneliner to get the $user:



        $user = User::with(['photos', 'interests', 'gifts', 'visits', 'likes'])->whereId($id)->get(); // if you stick with not using Route Model Binding
        $user->load(['photos', 'interests', 'gifts', 'visits', 'likes']); // if you use Route Model Binding

        // in view you will access interests for example $user->interests


        Now what about edge case if user is looking at their own profile, as I understand the only difference is to change variable $page_name, do it right in the view (I would do it that way):



        @if($user->id == Auth::user()->id)
        My Profile
        @else
        $user->name
        @endif


        So proper controller after suggestions would look like:



        <?php
        namespace AppHttpControllers;

        use AppUser;
        use Auth;

        class ProfileController extends Controller


        public function index(User $user)

        $user->load(['photos', 'interests', 'gifts', 'visits', 'likes']);

        return view('profile.my_profile', compact('user'));





        Packages



        Do not reinvent the wheel, for example for handling attachments / images use solution that is tested https://github.com/spatie/laravel-medialibrary and used by many!



        During development debugbar is a must.







        share|improve this answer















        share|improve this answer



        share|improve this answer








        edited May 31 at 9:25


























        answered May 31 at 9:00









        Kyslik

        1815




        1815






















             

            draft saved


            draft discarded


























             


            draft saved


            draft discarded














            StackExchange.ready(
            function ()
            StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f194343%2flaravel-usercontroller-design%23new-answer', 'question_page');

            );

            Post as a guest













































































            Popular posts from this blog

            Chat program with C++ and SFML

            Function to Return a JSON Like Objects Using VBA Collections and Arrays

            Will my employers contract hold up in court?