Junior Code Review: Better Routes, CRUDs and Validation

hello guys today we have another code review by kind of a junior developer but with a very noble project for requesting blood and checking blood for non-governmental organizations it's a simple laravel 8 project based on laravel ui as a front-end scaffolding so you can register fill in some form let's use fake filler chrome extension so we register we're in for the dashboard and then we can request blood check blood requests then there's a profile then we can donate the blood create so a few forms with validations and also with admin area to manage all of that let's take a look at the code and let's see what advice i can give as usual i start every code review to understand the project from routes and routes are public then there's author outs from laravel ui and then there's user group of routes and then there's admin group of routes let's start with here and here i immediately can advise that this can be shorter so instead of route get if the return is only view there's a separate function route view like this i think is the syntax route view index so you specify the url and you specify the laravel blade view let's refresh and see if it still works it does so same here route get home should be route view home with index as well not sure if it's needed because different two urls point to the same route but oh well that's shorter right next in a few of my previous code reviews i mentioned that but i want to emphasize again code formatting this route file is pretty hard to read so this is not formatted so indentation so route get inside of this then this line does it mean that this is a separate group no it doesn't because route group is here so i would separate it like this and then like this then it's more clear that this is a separate route group and on top of that we can use automated code formatting from your ide like in my php storm there's code reformat code and what it does it added a few spaces here added indentation here so it's a bit more readable next let's take a look at user and admin groups how we can reformat it a bit better because it's a different logic here so in this group we have admin middleware auth and then admin check is a separate middleware which we can find in admin check middleware so this one so that's for admin group there's a separate middleware for all the user group i would assume there's no middleware at all but in fact if you go to home controller the middleware is in construct so this is inconsistent in my opinion you should use middlewares in the routes it's more readable and then it's pretty clear which middleware is for which controller so you don't need to click every controller and check if it has middleware and in fact when i clicked one of them which one it was profile controller no requisition controller this one this requisition controller doesn't have middleware so probably the developer forgot that and to avoid that i would advise to have middleware here in the routes either for every line or better route group so let's do exactly that route group then we have middleware auth for all of those routes below then callback function and inside of that function let's copy and paste everything that we have below so like this everything inside of that is protected by auth and then we donate auth here right and then all of those controllers are protected by auth and in all of those controllers we can remove that construct method one by one i won't do it manually in this video so just a few examples we can remove those construct methods and this is even a bit more readable so these routes are protected by auth and inside of that auth another group is admin with separate middleware and i went through controllers and checked some things so one by one what i've noticed let's go to donation controller and let's see here in the index donation controller index looks like this so it's a page 4 list of donations and then here we have donation where user id is logged in user which is good but then we have with data and in that user donations index if we open index blade for user donation we have the data so table and then for each of the data or for else of the data that's great that they use for else but i don't like the naming of the variables so data and d doesn't mean anything imagine another developer taking over your code to help that blood donation initiative and they see that thing and they don't really understand what is that d what is that data so instead of with data and i see with data repeating all over the place in all the controllers you will see it a bit later so instead of with data i would have with donations and that variable good and then for else of donations here donations as not d donation and then d let's replace that edit find replace with donation like this replace all and this is much more readable next thing i want to discuss is profile so visually on the page you can see profile it lists all your profile things and then you can edit the profile and in the routes there's route resource for the profile and then edit and i thought why separate that resource with index and update and then edit separately it should probably be a route resource and edit should be a part of that but then if you think about it there's no resource to edit you're editing your own profile so there's no user id or any object it's working with the same user so in my opinion it should not be a resource it should be just two routes route get and route post and to be honest it's a personal idea i wouldn't make a separate page for list the profile i would immediately redirect to edit profile so click profile and then you'll end on that form so then we need two routes route profile edit and then route put profile it would be update method something like this i wouldn't perform that in this video but just to show you so something like this then we don't need only instead we do name profile update and it would be more readable so i would not use route resource here but if we go to profile controller what else we can do here of course we don't need that one anymore here again you see with data and another thing auth user you don't need auth user you don't need to pass it from controller because it's available in blade without any additional actions so if you go to profile show blade in here data again i don't like data you can do auth user like this auth user name and then replace all the data with auth user so let's do replace with auth user like this replace all and then here in the controller we don't need to pass anything at all like this and also a quick tip about route resource if you're creating resource controllers don't leave the empty method please do a cleanup after yourself so only the methods that are actually meaning something would stay in the controller so now it's full of empty methods and again i need to scroll down to actually get to the meat of the file and speaking of that meat so to speak of the file the main thing let's stop and take a look at update function and there are a lot of different ways how we can perform the same thing so this is correct validation for only one field and then updating the user and then push to be honest for a long time i haven't seen pushed used it should be user save i think and that is all correct but we can make this controller shorter in a few ways so you can see this is repeating username so all those fields this matches this for all the fields so first i will show you the shortest way using request all but it's a bit unsafe so we will improve that in a minute but for now the shortest way would be auth user update and then you pass request all that's it and then you don't need anything else so just one line for it to work you need to ensure that your user model let's open user model has fillable fields so all of those fields should be fillable and then you can use that mass update function or mass create function but as i mentioned it's a bit unsafe because request all takes everything that comes from post request and someone may perform that post request not from the browser from their api client like postman or fake some data and if they guess the name of some field of the database and the value so for example they want to guess what is the field for admin and there is a field is admin so if someone passes that in the request it would be updated as well so someone may make themselves an admin which is a huge security issue obviously so to avoid that i suggest to not use request all at any times instead you have two options first you can specify only so only and then array of keys which you can probably copy and paste from here let's try to do that so something like this let's see if it matches name email password password is not here email is not here so we leave only what we actually want so username mobile other contact blood group is donor last donated cannot donate until so cannot donate until this is missing but then we need to add that to the fillables as well like this and then we don't need donor id here and pin code id i think it matches now so eight fields here and eight fields here so that's one way how you can do request only instead of request all and this is safe then or there's another way you can use form requests so you can generate something like php artisan make request update profile request for example then instead of that request here we use update profile request so we use update profile request and then we turn that request validate into here and pass all the validation rules inside of that request by default by the way we need to turn that to authorize true and then rules should be name required and then we need to specify all the fields here from request only so we need to copy all of them from here and specify validation rules even if they are not required so come up with some kind of validation rule it could be optional it could be string something like that so let's make it string probably it's incorrect so its donor probably is boolean but just for the demo i will take that to string and then we can first we can delete this and obviously delete this comment and then in here you can use not request only and not request all but request validated validated method would take only the fields that are listed in the form request validation rules so it's not accessible to have his admin or add any other fields and then it makes our controller much shorter with only two lines of code so this is my favorite approach to have crud update or quite create so validation rules in form request and then using request validated to update it in one line now i want to take a look at the requisition controller and specifically at the validation so visually i can go to request blood which is requisition create form i fill it in with fake filler some stuff that i need to edit so mobile numbers should be 10 symbols from what i remember or actually let's try to fire the validation so validation alternate contact must be 10 digits but then if i do make them 10 digits see what happens i save okay they must be different okay so let it be different i save and then the error please check the form below for errors do you see any errors here i don't so there is some kind of mismatch in validation rules and let's try to look what errors are there so if we open requisition create blade i see that for every field there is an error section with blade which is correct so obviously some error is not shown although it appears and let's change that to show all the messages for that we go to laravel validation displaying the validation errors and there's a well-known snippet to copy paste it's for bootstrap but we don't really care what framework it is we just need to show all the errors and see what actually happened resubmit the form save and then the when wanted field is required let's investigate further so in the controller of requisition controller in the store method we have request validate with this validate array what is validate array some kind of array which is again hard to read because i need to scroll to the right so it should be at least something like this readable so all of those lines like this but my other question is why do you need that private validate array because it's used only once in store i thought there was update method to reuse that array but not the case update method isn't even used maybe it will be used in the future so that validate array should not be an array in the controller like private variable again it should be the same as i've shown just now form request with array of validation and the actual error here is if we scroll to the right let's search for when wanted one wanted required and if we search for that field name in the blade it's when wanted here and the actual error here is that if validation is fired for example again let's take a look here the value disappears so this input doesn't have old value here value should be old when wanted let's try to do it again let's pick one wanted some date then let's leave that validation error we save and this is okay now and if we change that we save then it's saved correctly so this is kind of a live investigation what could go wrong in validation you should check for all the cases of the validation so at least fill in the form yourself like 10 times with different options or write automated tests for different options and another thing i wanted to emphasize what i've seen in the same requisition controller in the show method so show off requisition if we click show here it's all working but it's loading too much data so return view requisition good with data which again should be requisition so with requisition like this requisition and then with comments this one we don't need to load with comments separately because we have comments here and in the blade then in the show of requisition of course we have data here but let's search for comments requisition parts comments here we should have comments somewhere yeah for else of the comments it should be okay to have requisition comments because we're loading that so no need to have a separate with comments here like this that should work and also what i found related to requisition comments the model tip for the relationship again empty line should be between the methods and then here those two variables they are not needed if you specify the relationship with correct well-known structure so user has many donations which means that the database field is donations dot requisition id and the local key is id this is not needed but then the relationship should be called not user donations but donations like this and then the same with comments requisition comment requisition id local id should work without those and then looking through all the other code everything is pretty much repeating all the things that i've mentioned so with data is everywhere all the crud things so i won't repeat myself for the admin area it's all pretty much the same and also i saw organization detail controller which is the same mistake or inconvenience as the profile so again route resource and then get so it should be probably edit and post methods instead of route resource so that's it quick review which turned out to be not so quick so i hope you stayed until now and if you want more reviews like this one i already have a list of people who sent me the repositories so don't send me more for now please have a queue of around five reviews that will be made in upcoming weeks and if you want to get them all subscribe to the channel and if you want to financially support me on this mission of daily videos and reviews of your code check out one of the three products that you can see on the screen from myself and my team laravel admin panel generator my courses which is 15 courses at the moment or set of live wire components at livewirekid.com see you guys in other videos

Channel: Laravel Daily

Views: 30,341

Keywords:

Id: ZigTkUispLw

Length: 17min 58sec (1078 seconds)

Published: Wed Mar 10 2021