Junior Code Review: Cleaning Up Laravel CRUD

Another junior code review, with 10+ random tips. I'm starting to feel that I repeat myself in different reviews, so I guess I will switch to topic-based reviews in the future.
00:00 Intro
00:41 Error: Duplicate Migrations
02:02 No Data - No Homepage
04:40 Home: Compact or NO Variables?
06:55 Controller Optimization
08:23 Global Settings
09:19 Admin User Route Group
09:54 Controller Naming
10:32 UserController and Other CRUDs
13:34 Storing Images and Thumbnails
Related articles:
Laravel: Sharing Data With All Views laravel.com/docs/8.x/views#sh...
PSR-2 and PSR-12: Why We Need Standards and How to Apply Them blog.quickadminpanel.com/psr-...
Group Code Review: Laravel Image Upload & Resize • Group Code Review: Lar...
Playlist for other code reviews:
• Junior Code Review: Be...
- - - - -
Support the channel by checking out our products:
- Try our Laravel QuickAdminPanel: bit.ly/quickadminpanel
- Enroll in my Laravel courses: laraveldaily.teachable.com
- Purchase my Livewire Kit: livewirekit.com
- Subscribe to my weekly newsletter: bit.ly/laravel-newsletter

[@bcmarcos03]

Hello good day. I am a SAP Developer with more than 10 years of experience. And I have started with laravel 1 year ago. When I'm developing in Laravel, I don't have "logic" issues, but "best practices" issues. And I get stuck trying to find the best way to do something. Your videos are helping me a lot in that matter. I find your trainings very interesting and helpful. Thank you.

[@steamkocheaterreport]

Im on same page. Constantly asking myself "is this the best way to do this?" when code already works... Another one being absolutely terrible documentation, not the never ending texts for reading like lara-docs provide but actual documentation for functions available. And yet another one being terrible code suggestion support where i know function exists but vscode or other editor just dont suggest a thing.

[@victordivo5972]

All of the review are so amazing, never get bored when watching your video

[@bijportal5643]

My New favourite channel on KZbin 🔥🔥 especially these code reviews 🙏🏾

[@remyjay1509]

This is a great series, I get so much value from it. Also, I do like the idea at the end of focusing on a specific type of refactor on each episode. That sounds like a fantastic direction to go.

[@hungcuong5574]

Thank you, Povilas. Your code review videos are very educational.

[@asatbekxalimjonov4005]

you are right

[@Steviec63]

Thank you for the Spatie Media Library tip!!! I've just watched their tutorial. It is amazing.

[@mctiggaz]

Hey this is my student! In my opinion he is pro not junior :) I’m glad that I have learned him so much. Greetings from pingdevs dot com, Macedonia.

[@NB-ph6cv]

Bravo!

[@alicenNorwood]

You'll probably be a man of the year in a laravel community soon

[@codecreeper]

cool I didn't know about the spatie media library ! Thanks for that ! ^^ I will use it for one of my project !

[@JouvaMoufette]

Another thing I noticed very briefly: Unless I missed another use of $staticpages in the homepage view that needed all staticpages, the loop around $staticpages was calling ->take(4) which means they were querying ALL static pages, but only needing 4. If that is the case, you should not be using a straight up all() and instead be using ->limit(4)->get() or ->take(4)->get() Getting all then only getting 4 from there is VERY expensive on the database. And databases in scaled applications are often not on the same server as the web server, meaning there's a larger load on the network now.

I saw that too. Also, he/she's doing select * and use only 2 property in the view.

[@Rocknrolla112]

repetition is the mother of learning ))

[@pvaitonis]

Great idea, nice job.

[@thisisthelogic]

Povilas your job is awesome! You could do a more advance code review too? A thank you from Brazil!

[@LaravelDaily]

I sometimes do advanced reviews, thanks for kind words :)

[@sakchanno601]

I' m realy like your tutorial.

[@arturkhachatryan63]

Thank you!

[@wahyudi2916]

because your videos... php storm is become my next IDE :D

[@themrflibbleuk]

Not worth the money, I’m all for Sublime.

[@jakubkooooo]

Hell no, IDE >>>>>>> Text editor (even with ton of plugins)

[@themrflibbleuk]

I will say for the low cost of Sublime, I get very close to an IDE at a much lower cost. You can also argue, without every IDE function you learn and remember more.

[@Steviec63]

PHPStorm is the best IDE. It has a seemingly endless number of features. They add more all the time. The more you use it the more you start using the added functionality. Sublime is good but only if you want to save money. If you really want to save money use Notepad++ :)

[@themrflibbleuk]

@@Steviec63 Notepad++ and Sublime are worlds apart.

[@rporter75]

Quick note about the first issue, rather than having to rename the class and file name, you could just anonymise the migration class: `return new class extends Migration`. I believe in newer Laravel versions this is actually the default so my note may be a bit outdated 😅

[@adebajooluwaseyi2124]

wonderful video

[@user-xq5gp1mk5b]

Hi from russia. Its very useful, especially for me. Keep it up, your doing well 👍🏻

[@vimkndll4171]

Which programming language is mostly used in Russia bro

[@user-dv9fk1hd3s]

@@vimkndll4171 Same as everywhere

[@LoganathanNatarajanlogudotcom]

Thanks sir

[@jersoncarin1952]

nice explaination

[@Raysierer]

On 12:40 he could use inside the function (Request $request, User $id) instead everytime to FindOrFail the user id

[@doremidon]

Thank you for video. 9:36 you deleted "check.user:Admin". But all users still can register and right a way have access to admin panel!!! Should to prevent other user to have registration on system or not delete "check.user:Admin" middleware ;)

[@doremidon]

For this actioon code like " Auth::routes(['register' => false]); "

[@LaravelDaily]

Hm, interesting, I was debating it with myself, because there are no actual routes for non-admin users, so it doesn't make sense to have admin routes/middleware without non-admin routes. But technically, I agree with you, you're right.

[@mctiggaz]

@@LaravelDaily during the class I’d wanted my students to learn about middleware also so keep it mind that this student switched his carrier from to programming. For only 6 months he learned everything and made this cms for only 2 months. He is good! :) I wish him successful carrier.

[@firefoxo]

What Laravel admin dashboard would you recommend? I've used Vuetify in the past, and I liked it. I was looking for something more lightweight, but with VueJS functionality.

[@marczello8162]

8:36 its shorter way to set this variables, you may use Constructor Property Promotion (new in php 8): public function __construct() { private $settings = Settings::findOrFail(1); }

[@codecreeper]

thanks

[@yezperdk]

0:41. I like to follow the RoR-way of naming migrations. E.g., if I want to add the field `name` to the Contact model, I will call the migration AddNameToContact. That way, there should be no collisions, and the file name very clearly describes what the migration does.

[@andreich1980]

Unless you add a field then decide to delete it then add again ;)

[@yezperdk]

@@andreich1980 Haha true! Guess I haven't had that need yet :)

[@deeploy]

cool I didn't know about the spatie media library ! Thanks for that ! ^^ I will use it for one of my project ! I would like to know if you are able to talk also about scoping in Laravel. I've been in a company that have a senior developer, and use a loooot of scopes through the hole project. And he never uses repositories (just one or two), even so it was fully needed because of the bunch of requests he made in model. I like the repository design pattern for complexe relationships, I often use repositories (when it's necessary of course). But do you have a simple example when using scopes, when using repositories ? And by the way, could you tell also what is the difference between a trait and a helper ? ^^ Thanks for reading me, and keep going, I love to watch your videos :)

[@LaravelDaily]

1. Scopes and repositories are kind of different things, two different topics, you can use scopes without repositories, repository pattern is misused very often in Laravel and therefore I don't like it very much. Search my channel for "repositories" for a few videos. 2. Trait examples from my new project: laravelexamples.com/tag/traits Helper examples: laravelexamples.com/tag/helpers From those, you should understand/feel the difference.

[@andreich1980]

latest() orders by created_at date, not by ID. In this case it might be fine but generally it's wrong. It should be latest('id') instead.

[@munthirzikre2401]

I think the result is the same for both

[@andreich1980]

@@munthirzikre2401 In this case yes. But what if they create a record with `created_at` in the past, for example. If 2 bits of code return the same value doesn't mean they work the same, you are just lucky enough at the moment.

[@A1s2d341673214]

The created_at and id, follows the same order. I guess.

[@llBestBoyll]

gg man 🔥👌🏼

[@enigmatics-lives]

The ‘=‘ argument in where() is not required, but occasionally not writing it may cause an error, that will be hard to debug. If search term in where will be “=“ it will cause an error of Builder, for not writing the third parameter. Of course, it will happen with input from user more likely, but, who knows.

[@phanta5m]

if the table created by laravel migration the '=' may not be needed. but if have table before, the '=' are very needed. based on my experience. CMIIW

[@GergelyCsermely]

Hi! I'm enjoying all your coder reviews. Question with validation: I started to do things with LiveWire. In the docs of LiveWire "You might be wondering if you can use Laravel's "FormRequest"s. Due to the nature of Livewire, hooking into the http request wouldn't make sense. For now, this functionality is not possible or recommended." What do you think? Is more secure or has better performance do the backand things without Livewire and use FormRequest and validated type of checking the Request data or can I do with LiveWire without any issues on long term?

[@LaravelDaily]

It's a personal preference, so whatever you prefer, depending on how you want to use Livewire - as a full-page component, or just as a part of the form for some fields. There's no security/performance difference, it's about structuring Livewire components how you want.

[@GergelyCsermely]

​@@LaravelDaily Thanks. For me is more elegant your solution, but is not supported from LW. :(

[@Jurigag]

6:48 he selects all static pages but as i understand method take he only uses 4 of them? So he should select only 4 static pages in controller, not all.

[@JouvaMoufette]

Yeah there's a big big difference between ->take(4)->get() and ->get()->take(4) and it'll be especially noticeable in production as the product grows and has more entries, and starts doing things to scale like having a database on a separate host and now all of the records have to come back as network traffic

[@winmodif]

Thx for the video! Could you or someone please help me with the following; I have a 'catch all route' with "->where('menu', '.*')" and in my controller i'm loading in a bunch of variables though not all variables are nessecary for each single page. It depends on certains fields and/or relationships. I'm pretty sure there is a way to conditionally load in variables based on that. Could you point me in the right direction please?

[@anisurrahman6634]

nice

[@devsbuddy]

Actually I create trait "CanUpload" in every project for file upload and use whenever and wherever I need it.

[@empty7999]

Hello Mr Povilas , first thank you so much for your great content , my question is do we need csrf token when using LiveWire ?

[@andreich1980]

We don't need to do anything with it. It just works.

[@empty7999]

@@andreich1980 Thank you so much Andrew ✌🏻

[@sanchopansa7732]

Probably it's worth noticing that during development it doesn't make sens to make additional migrations just to add a new field in the table. It wouldn't make more sense to just add those fiels in the original migration and be done with it? PS I am just starting with laravel so please correct me if I am wrong.

[@LaravelDaily]

If you're working alone by yourself and locally, and if you can afford to run "php artisan migrate:fresh" on every change, re-creating the whole database, then yes, it's ok to edit older migrations. But as soon as you have real data, or you deployed project on some other server where other developers work, it's better to create new migrations.

[@mohammedrizwan8123]

3:57 which chrome extension is it??

[@joaoprizal7420]

Hey can you make a video about Porto pattern (apiato) ? Thanks

[@LaravelDaily]

Not in plans currently, sorry.

[@alila3883]

good

[@pauloclara4764]

hello .. what's do you use to fill de forms with data? thanks

[@merkurijus5317]

Fake filler chrome extension

[@pauloclara4764]

@@merkurijus5317 thanks

[@brunomaximo7361]

find(1) to First()

[@shreydadhaniya2587]

Can anyone explain me that how can i set up project2.test url for my project

[@LaravelDaily]

It's done locally by my Laravel Valet server

[@rehabragab5937]

Great tutorial,may I send my code for review?

[@LaravelDaily]

Currently I'm done with junior code reviews for now, but I will start PUBLIC code review program next week. So watch the new videos on this channel on Monday-Tuesday and you will get all the details.

[@1234matthewjohnson]

Thank u very much. Can i ask what SQL tool you use?

[@travholt]

Sequel Ace, I believe. github.com/Sequel-Ace/Sequel-Ace

[@laithalenooz7082]

+1

[@GergelyCsermely]

Check the DataGrip made by Jetbrains. I think it is the best from the MySQL admins what i seen before.

[@LaravelDaily]

Sequel Pro

[@necmttn]

Awesome content, Thank you! Would be nice to have an example for the Laravel project which has both blade front end and also sanctum CRUD API access from other places. public function store(CreateJobRequest $request) { $job = $request->validated(); $job = new Job($job); $job->author()->associate($request->user()); $job->save(); if($request->expectsJson()) { return $this->success($job); } return redirect()->route('job.show', $job->slug); } right now i'm doing this but not sure it's the best way of doing it or not ?

[@LaravelDaily]

It depends on the project, but in my experience, if the project is both web and API, they should be separate in different controllers, web and API ones. But again, it depends on the project, if your method works for you, use your method.

[@debjit21]

I liked the admin theme, What is it?

[@chengkangzai]

if im not mistaken, its material design bootstrap

[@mctiggaz]

Material design bootstrap, free version.

[@jsjunior]

PHPStorme theme ?

[@LaravelDaily]

Material Darker

[@pippop9583]

Call a function in array declaration is not a good idea

[@dfordemo981]

2:53 what is the name of this mysql software?

[@travholt]

Sequel Ace, I believe. github.com/Sequel-Ace/Sequel-Ace

[@dfordemo981]

@@travholt thanks, i'm windows user 🙁🙁

[@LaravelDaily]

Sequel Pro

[@dfordemo981]

@@LaravelDaily thanks

[@iamriwash7943]

He can put that setting in provider using blind

[@koussayhajkacem287]

The email address of the guy is in the address bar...

[@LaravelDaily]

Yeah, missed it, edited it out now. Thanks for the notice.

[@sloppyprogrammer4373]

find the declaration of $data = [ ]; in each controller quite pointless when you can just do something along the lines of view('view')->with([ '' => '']) or view('view', [ '' => '' ]) It's not really proper handling of (large) view/result/response data collections anyway, but if you must really make logic like this just keep it simple and pass all finals inside the ->with() method or directly inside the view() $data parameter, instead of declaring a variable you're never going to modify in the first place. I think it's bad altogether to generate a lot of response arrays inside the controller, while usually you can segregate this into a defined object, but in some situations something along the lines of this is acceptable since it's easy to read and easy to identify it being a final/result collection of data. Also it is accepted for very generic controllers that have next to no logic to split up into an object of it's own and where you want to keep this logic inside the controller since it can invoke unnecessary complexity if you decide to OOP when you don't have to OOP. // Note don't declare a variable for result collections if it's behavior won't change $data = [ 'posts' => Post::all(), 'meta' => MetaData::render(), // ... many other things ]; // While scanning the code I'm expecting something to happen here, but nothing happened... return view('view', $data); // A little bit better.. It is short, easy to read : but keep track of the complexity of your controller and consider making a service return view('view', [ 'posts' => Post::all(), 'meta' => MetaData::render(), // Shouldn't be served from here, consider view::share() // ... many other things ]);

[@marcusgaius]

While you are definitely providing useful information to many, I think you shouldn't be doing QA for code that hadn't been tested even once. Speaking of site settings/config, I would appreciate your take on making such a package, or if you already have one in mind, could you mention it in a reply, please. Keep up the good work. 👌🏻

[@LaravelDaily]

Why shouldn't I be doing QA for the code if a person asked for it himself? Regarding settings/config, this one is good: github.com/spatie/laravel-settings

[@marcusgaius]

​@@LaravelDaily My bad, I guess I had a different idea about what the series was about. The information provided is very valuable, I just think that you had to say "I won't repeat myself" about different things too many times in this video. That to me indicates the time used on making this video could've been better spent, but that's just my perception of it. And the impression I got from your reactions. NHF, of course. Thank you very much for the link.

[@rajabhishek2936]

Awosem content

[@HadToChangeMyName_YoutubeSucks]

I'm working with some legacy code right now (procedural php that's 15 years old, not laravel) and the programmer not only put retrieved all data and put it into variables, he put them into variables named differently than the data being put in so signTypeID is put into modelID, then he checks the data and puts it into an array in an array element named model. That's then put into a form element named signID, when it's saved it's put into a json variable named modelNumber and at the receiving code it's renamed multiple more times. I have no way to know what data I'm working with at any time. That's just a single actual example, he did this with practically every pace of data pulled out of the database. I actually had one function that was over 3000 lines of spaghetti code littered with renamed data, if thens, repeated manipulations just copy/pasted instead of calls to a single function that did the work, no case switch statements where they were obviously begging for them. It's actually kind of impressive that it even worked, though it worked poorly. The sound of...let's call it frustration...coming out of my office is pretty constant. Don't do it people, for the sanity of the guy who has to come after you and fix your code.

[@donthatedonate]

Hey! What Phpstorm theme and color scheme is this?

[@LaravelDaily]

Material Darker.

[@Paltibenlaish]

is not good to do ::all(), maybe some eager loading ? optimization problems ahead ....

[@olehtalanov6697]

You should not rename migrations this way!

[@LaravelDaily]

What would you have done in my situation, where migrations just don't work and are in conflict?

[@travholt]

It would be helpful if you explained why.

[@laithalenooz7082]

+1 Explain please

[@firefoxo]

Migrations are also stored in the database, so if you change the file name, you break the migration I think.

[@karlebh]

@@firefoxo if you refresh your database, the migration files get rewritten.

3:56 he/she is not validating.

[@micosair]

It feels to me like the junior is kinda of an a-hole,trying to get you to fix his sht or just trolling to waste your time.