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.
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.
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.
Thanks. Plz make more like this. Code review is good place to learn. BTW Im from Russia, I think you have the best laravel TH-cam channel in the world!
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.
@@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 ]);
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); }
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.
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; }); }
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.
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 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.
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 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)
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.
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?
For controller method ill prefer to make variable $data = $request->validated() in start of controller method. If(image) { logic $data['image'] = $name }
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.
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();
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
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.
th-cam.com/video/twPM-6X0pMI/w-d-xo.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?
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.
![SAFEPLANET - ดาวเคราะห์ [ THE PLANET ]](http://i.ytimg.com/vi/4X1XL3I_k3c/mqdefault.jpg)
In my opinion, this kind of video are the best for junior level developers, like me, to learn from others mistakes. Thank you.
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
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
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
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 keep doing the junior code reviews. Helps a ton!
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, as a junior developer this video really help me how to code better
Thanks for the video! Feeling myself much more confident as a Laravel beginner!
Please make more of this code reviews. Learnt a lot in just one video. Thank you
This looks more like a person who just started laravel vs even a junior. Good review!
what is you definition of junior ?
Thanks. Plz make more like this. Code review is good place to learn.
BTW Im from Russia, I think you have the best laravel TH-cam channel in the world!
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.
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.
Thank you! These are extremely helpful. Looking forward to future code reviews.
we learn a lot please continue making more video like this in the future
This really helped reduce my imposter syndrome. I can sleep peacefully tonight unless I read a freelancing project requirements
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.
Nice video,
I'm currently having multiple projects using Laravel, I currently use Laravel module package to modulate my application
Truly helpful indeed, such lesson is what I need :) Thanks for your hard work.
Thank you for taking your time and teaching us this valuable information.
extremely useful for Laravel newbies
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
With admin prefix, he can also create a new admin.php file on routes folder and separate admin routes from other users.
thanks for the review we need this type of video to learn how to code
pleaase make more review videos on your channel
Thank you Povilas... Great Review!!!
thank you from Zimbabwe
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.
I think in following the single responsibility principle, the code block to upload an image should be abstracted and just called when needed.
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.
Yessss! Code review! thanks
Worked with Laravel Love - very powerful package.
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.
Thank You, I like this videos, excellent help to learning
How did you put sidebar as an icon in Phpstorm? @Laravel Daily
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.
this kind of video is very very helpful
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)
thank you sir it's really helpful please keep it up
Love this video. thanks
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)
Thanks a lot. Will you make a video.... How to delete database notifications by row in laravel?
Thanks as always!
Good explanation!
wonderful video
if you follow junior code review as a series It would be appreciated, as well please make records on php design patterns
Best regards
Should I import the namespace of the class I use in route files?
Thank you, very helpful
Thanks, the tips were great!
Thanks so much for the video
Helpful AF
Hi , please tell me how can i get ur level ,
I'm lost
What should i do ?
thanks for sharing
Exactly what happens when starting in a new company. Repositories without any instructions on how to install and configure things.
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.
Nice 👍
Sorry, but when I repeat the code I see that it is better than doing 2 sql
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: th-cam.com/video/-77LnBSL2Zk/w-d-xo.html
Thanks
For controller method ill prefer to make variable $data = $request->validated() in start of controller method. If(image) {
logic
$data['image'] = $name
}
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.
I just wonder when will I be this good with Laravel 😢
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.
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
Things to do before laravel application deploy
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.
Would you "Junior code review" my code, please.
Maybe, if it's not a big piece of code. Send it to povilas@laraveldaily.com with information what exactly I should pay attention to.
"I got used", proceeds to make a mistake.
omg, how to get the password? guess?
Crack!
th-cam.com/video/twPM-6X0pMI/w-d-xo.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.
its not disrespect at all, its still a junior code so you expect these kind of errors lol
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.