Laravel Junior Code Review: 12 Tips on Everything
Вставка
- Опубліковано 14 лип 2024
- After quite a while, I'm making another junior Laravel code review, with tips on migrations, routing, PSR standards, Route Model Binding, and more.
00:00 Intro
00:40 Make Migrations Work
02:59 Seed Admin User
04:48 Short Project Demo
05:16 Routes: Laravel 7/8 Way
06:24 Method Names: PSR-1
07:01 Extra Routes Before Resources
07:35 Separate Admin Controllers
08:52 Likes/Unlikes - Package?
10:01 Route Model Binding with Slugs
11:29 Middleware in Routes
12:22 Weird protected $recipe
13:43 DRY Controller Code
Related links:
- PSR-1: Basic Coding Standard www.php-fig.org/psr/psr-1/
- Laravel Love: Likes and Dislikes for Eloquent Models: laravel-news.com/laravel-love
- 4 Ways to Auto-Fill Field Value in Eloquent • 4 Ways to Auto-Fill Fi...
- - - - -
Support the channel by checking out our products:
- Enroll in my Laravel courses: laraveldaily.teachable.com
- Try our Laravel QuickAdminPanel: bit.ly/quickadminpanel
- Purchase my Livewire Kit: livewirekit.com
- View Laravel Code Examples: laravelexamples.com
- Subscribe to my weekly newsletter: bit.ly/laravel-newsletter - Навчання та стиль
Thank you for review. I have to apologize for the code was not working, (migrations issue) and the readme files. I didn't mean any disrespect. I forgot on these important things. My fault.
Is your code anywhere in GitHub? I'd like to clone it
In my opinion, this kind of video are the best for junior level developers, like me, to learn from others mistakes. Thank you.
Please make more of these code reviews because it offers many things to learn at once especially how we should do things. Thank you.
I love the code reviews , I always learn a lot , thank you
Please keep doing the junior code reviews. Helps a ton!
After watching all the review code series I feel like i am a junior developer even with few years experience :D. Its more effective way to learn indeed. Thank you for making these efforts. appreciate that.
These videos are great because it helps enforce standards in coding and shows new devs the standards that they themselves may not be aware of
This is great, I love this kind off video to see how others think or approach things help me see things from different angle
Thank you! These are extremely helpful. Looking forward to future code reviews.
This looks more like a person who just started laravel vs even a junior. Good review!
what is you definition of junior ?
We learners are really feeling blessed to learn daily from you - god bless you.By your laravel tips,series and junior code review i have learned and still being exited watches your every video.
Please make more of this code reviews. Learnt a lot in just one video. Thank you
Truly helpful indeed, such lesson is what I need :) Thanks for your hard work.
Thanks for the video! Feeling myself much more confident as a Laravel beginner!
Thank you for taking your time and teaching us this valuable information.
Nice video. Just one suggestion. For the DRY Controller Code, though the code is becoming shorter it adds the double query to the database. May be it can be made efficient by using an array and then in the if condition adding the image key-value pair in the array and at the end create statement can come.
Something like
public function store(RecipeStoreRequest $request)
{
$recipeArray = [
'title' => $request->title,
'category_id' => $request->category
/* other key=>value pairs */
];
if($request->hasFile('image'))
{
/*code for file upload*/
$recipeArray['image'] = $name;
}
$recipe = auth()->user()->recipes()->create($recipeArray);
}
Yes, I agree, good catch.
we learn a lot please continue making more video like this in the future
Thanks. Plz make more like this. Code review is good place to learn.
BTW Im from Russia, I think you have the best laravel UA-cam channel in the world!
thank you, as a junior developer this video really help me how to code better
Yessss! Code review! thanks
Thank you Povilas... Great Review!!!
With admin prefix, he can also create a new admin.php file on routes folder and separate admin routes from other users.
This really helped reduce my imposter syndrome. I can sleep peacefully tonight unless I read a freelancing project requirements
extremely useful for Laravel newbies
Thank You, I like this videos, excellent help to learning
Thanks, the tips were great!
Nice video,
I'm currently having multiple projects using Laravel, I currently use Laravel module package to modulate my application
Thanks as always!
thank you sir it's really helpful please keep it up
thank you from Zimbabwe
thanks for the review we need this type of video to learn how to code
pleaase make more review videos on your channel
this kind of video is very very helpful
Love this video. thanks
Good explanation!
Thank you, very helpful
Thanks so much for the video
You could atleast two reviews per week. It doesn't only helpful for junior developers,but it helps keeps more advanced devs in line and current
Well, send me the code to review :)
Not to disrespect but, I'm already appreciating that he uploads even one code review.
Worked with Laravel Love - very powerful package.
For the recipes create/update operation I get his point for the duplication. He doesn't want to run queries twice. Your approach is okay. But I'd prefer to "make an array of common attributes, push conditional attributes to that array" and then create the row. With this approach, I'd need to run create the row only once and no duplication.
I was thinking the same thing but I can do one better. There's no need to merge arrays even. If you add the create after the if statement for dealing with the image. The code uses the variable $name (though I'd call it something clearer like $image_name) but you can just include that in the array, if there was no image uploaded that variable won't get set and it will just be null.
@@ryanb509 I am not sure if that's a good approach or not, because you're trying to access a variable that's not even initialized unless gets into the if block. and I don't think it actually differs from my approach.
$attributes = ['a' => 'a', 'b' => 'b']; // add the common fields
if (logic) {
$attributes['images'] = 'value';
}
Model::create($attributes);
IMO, the above snippet looks what it means and I think your approach doesn't add any value. That's my opinion.
Yes, I agree, good catch.
@@sirajul-anik Both work. IMO I find the code for my method a little cleaner and more clear. For example if you are trying to quickly look into what is being passed into create, its right there. However with your method you have to go back in the code to where you are setting the variable then also look at where you are merging in the image to get the full logic.
if ($logic) {
$image_name = 'value';
}
Model::create([
'a' => 'a',
'b' => 'b',
'image' => $image_name ?? null
]);
@@ryanb509 it's more of the developers taste. both works fine.
Helpful AF
Thanks a lot. This is really very helpful. I just remembers I shared my codes with someone but forget to share the login details! and I know how irrirating it can be.
thanks for sharing
Image can be stored like this:
'image' => request()->hasFile('image') ? request('image')->store('images/recpies') : null
So the final code can be even cleaner and we don't need to use if-else and update record after it has been stored.
$recipe = auth()->user()->recipes()->create([
'title' => request('title'),
'category_id' => request('category'),
'procedure' => request('procedure'),
'duration' => request('duration'),
'ingredients' => request('ingredients'),
'image' => request()->hasFile('image') ? request('image')->store('images/recpies') : null
])
Thank you for sharing this code. I really needed this. I need some help in my code, can you pls help me out.
I have Leaverequests table,
$table->foreignId('leavetype_id')->references('id')->on('leavetypes')->constrained()->onDelete('cascade');
$table->foreignId('user_id')->references('id')->on('users')->constrained()->onDelete('cascade'); //User requesting leave
$table->foreignId('handover_staff_id')->references('id')->on('users')->constrained()->onDelete('cascade');
$table->foreignId('approved_staff_id')->nullable()->references('id')->on('users')->constrained()->onDelete('cascade');
ListLeaves Controller,
$leaverequests = Leaverequest::with('user')->latest()->paginate(5);
I am unable to get the username for "handover_staff_id" and "approved_staff_id". I am already getting the "User requesting leave". I am unable to create the relationship between User modal and Leaverequests modal. Thank you in advance.
The "duplicate code" refactor calls the database twice. What I will do instead is to construct first the array of parameters. If there's image then I will add it to array of parameters. Lastly, I will call the create method passing the array of parameters.
I think in following the single responsibility principle, the code block to upload an image should be abstracted and just called when needed.
Nice 👍
You can get the data out of request first then using it to check if the is image in the request or not, then after that insert into the database. This will using only 1 query instead of 2 in case there is image. I was taught that limiting the db transaction as much as possible.
Thanks
if you follow junior code review as a series It would be appreciated, as well please make records on php design patterns
Best regards
Thanks for the review!
The slug attribute must be unique for each recipe because it is used as a key to get the recipe in the route model binding. However, in this implementation, if two titles are the same then there will be conflicting slugs because the slug is not unique at the database level.
Good catch.
I found this solution:
protected static function boot()
{
parent::boot();
// registering a callback to be executed upon the creation of an recipe
static::creating(function($recipe) {
// produce a slug based on the recipe title
$slug = Str::of($recipe->title)->slug('-');
// check to see if any other slugs exist that are the same & count them
$count = static::whereRaw("slug RLIKE '^{$slug}(-[0-9]+)?$'")->count();
// if other slugs exist that are the same, append the count to the slug
$recipe->slug = $count ? "{$slug}-{$count}" : $slug;
});
}
I would like to use spatie sluggable package to prevent such issues
Thanks a lot. Will you make a video.... How to delete database notifications by row in laravel?
I think in last example better to do this:
$recipe = [all data in example];
Then if (image) $recipe['image'] = value;
And then user()->recipe()->create($recipe)
This optimize query to db, I think 🤔
You don't need create, then update, if you can once create)
Hi! Thanks for video! I think last case was wrong way of solve because in corrected version gets 2 request to database first for insert and second to update which is not cool! There have another ellegant way to solve it)
How did you put sidebar as an icon in Phpstorm? @Laravel Daily
Exactly what happens when starting in a new company. Repositories without any instructions on how to install and configure things.
For controller method ill prefer to make variable $data = $request->validated() in start of controller method. If(image) {
logic
$data['image'] = $name
}
I remember my code looking exactly like that when I started using web frameworks, especially Rails and that habit of mistakes was brought in to my first Laravel project. lmao
Should I import the namespace of the class I use in route files?
I just wonder when will I be this good with Laravel 😢
Hi , please tell me how can i get ur level ,
I'm lost
What should i do ?
It is necessary to declare the reference between fields in migration or we can put a simple integer
and it can be done through a relation in the model. For example if i had relation between recipe and user from function
that is a wrong practice?
Watch this video: ua-cam.com/video/-77LnBSL2Zk/v-deo.html
Things to do before laravel application deploy
Thin Controllers Fat Models, do this principal applies for LARAVEL controllers also, because most of the time, I see thin Models and FAT controllers in laravel.
Crack!
"I got used", proceeds to make a mistake.
Can the last tip be written in a better way by using save() method instead of create()? Smth like this below
$recipe = auth()->user()->recipes();
$recipe->title = $request->title;
..... Other fields.....
If ($request->hasFile('image')) {
$recipe->image =.....
}
$recipe->save();
Probably, but not sure it would be worth this change, in fact it makes it less readable, in my opinion.
Yeah, I did the same too, because it only requires one connection to the database
Yes, I agree from DB query perspective, good catch.
Sorry, but when I repeat the code I see that it is better than doing 2 sql
The change you made to deal with the duplication doesn't look right to me. The original condition checks only duplicates because the coder didn't realise they could append to the array that gets passed to create(). I get that you fixed it by adding an update if an image was specified, but you created a new problem by adding a sql command that doesn't need to exist. I'd instead encourage prepping the data properly before sending to the db to be more efficient.
omg, how to get the password? guess?
disagree with the "duplicated code thing". you're trading couple of lines of code into 2 separate db queries which may become quite costly in some huge distributed db servers and cause unneeded delays. could just create an array first, and then based on conditions add an 'image' key for that image and still do a single 'create' thing.
i see those issues all the time when some trivial data is being injected based on couple of different relations, and it's one insert followed by 4 updates of newly created record. imagine you will need to add some versioning/revisioning or custom logging on top of that, when "date_updated" changes 5 times in 2 secs and 5 records in the "actions log" db. hope it helps some new coders - always think about scaling and optimization. not about 8 lines of code instead of 10.
ua-cam.com/video/twPM-6X0pMI/v-deo.html
Thank you but I have one comment for that
You saved the recipe with query
then if it has image you did another query
You hit database two times (2 queries)
I think there is another good approach for that how about?
Yes, good catch,.other people also pointed it out in the comments, read other comments on this video.
It feels sloppy to just send code that isn’t working, I agree a bit disrespectful if you will.
In the RecipeController@index
It's not great implementation for view_deleted... They don't eager load category and user for trashed recipes...
It should be like this
$recipesQuery = Recipe::with(...)->where(...)->anything(...)
if (/* view deleted condition */) {
$recipeQuery->withTrashed()
}
return $recipesQuery->get()
Or something similar.
RecipeController@store
They could get current user form $request->user(), no need to use auth() helper
Save an extra query
$recipe = ...make()
fill(['image' => ...])
$recipe->save();
It's a bad way to store images. They should have a recipe_id prefix or be stored in the recipe_id folder (123_name.jpg or 123/name.jpg)
There's a tiny chance that 2 different users create a recipe at the same moment and there'll be a collision.
Laravel has a default behavior - it generates a random file name - this is better.