Junior Code Review: Laravel Routes, Middleware, Validation and more

hello guys today we'll have an example of refactoring of the laravel code of junior developer which i will transform into a bit better code and it's kind of like experiment video so backstory locally in my country lithuania i started some kind of a mentorship program for junior laravel developers and part of that mentorship program is taking a look at their code and making it better provide any advice or help them make that code better you can call it code review in short so in this video i will make a code review of one of my students i won't name him it's just someone's code and let's make it better disclaimer i'm not here to scrutinize the code to blame someone for something that's the learning process the code is not bad it's working so it does the job for the client so pro tip for you juniors don't be afraid to show your code and pro tip for seniors don't attack the juniors that the code is bad we all started somewhere and disclaimer number two my advice is just my personal advice how i would change that code it's not necessarily the holy grail or the only way that's the beauty of coding and of laravel in particular that there are multiple ways how to do the same thing so let's go what is the project it's a very small kind of admin panel for managing materials so simple crowd of materials and you want to create a new you get a model window and then you save the data now let's take a look at the code and i will give around 10 tips or so so this video may be longer than usual on this channel but i think it will be really useful and very practical so let's start with routes and there are a few so called cruds so managing off production orders managing of material orders and materials themselves and first thing i would do here in this file is introduce route groups so if we take a look at this url and similar ones names are pretty similar and middleware is actually the same so that's the first thing so we can do route group parameters of that group so middleware equals role meister and then callback function of all of those routes now those routes become part of that group cut paste and then we don't need the middlewares here so that's already shorter each line is shorter let's delete all of them probably there is a quicker way to do that in phpstorm okay so that's a bit better next you can see the prefix of the url is the same so we can also take it out into a group parameter so prefix equals production orders like this and then we can remove it like this so that's shorter and probably we can transform that get and post into a route resource so it's hard to guarantee 100 up front but i do feel that there's index there's delete update form and update post and then create post and in route resource there are typical seven verbs here's laravel documentation for it so that's the standard of the urls for all those actions with route names so i guess instead of all of that we could do something like route resource production orders production order controller class production order controller class something like this currently as the code is written it wouldn't work because the naming of the methods and their behavior is different and i'll probably shoot a separate video of how to transform that but for now let's comment it out this is just a suggestion to transform five rows into one row with a more typical standard well-known structure and even if you don't want to transform those into route resource those verbs are pretty important actually so for example url delete should not be a get resource it should be route delete similar for update there should be route put it's kind of a standard that code would still work but if you want that code to be taken over by someone else so if you work in a team it's better to stick to standards next let's talk about middleware so i see middleware rolemeister for all of that group and in another group material orders there's role admin and of course we should introduce route group here but let's take a look at middleware's role admin and role meister i understand there are roles here so roll admin and there's also role manager middleware so like this and rollmeister okay so i see all of those middlewares are really similar checking the rows from user which is fine but i think it's too much coding here let's try to shorten it so let's take roll meister for example if auth check this sentence i would move into route groups so in routes web what i would do i guess all of those routes are accessible only for logged in users except for login form right so route group middleware auth then callback function and then inside of that cut and paste everything else so now all of those are protected by auth and then inside of the middleware you don't need to check that at all you just assume that it's already a logged in user so already shorter then user find author id you don't need to do that because it's already in your auth user so auth user like this will contain all the object full object of logged in user including the variables the fields so we delete that line and do something like this and it's already shorter let's click reformat code in phpstorm and it's a bit prettier probably this line is not needed also also this line could be shortened you see the same variable being checked so what if we do if in array role id and array would be three or one row three or one like this delete the other part and another bracket and probably one and three would be better so even shorter and then general thing about middleware probably return next request if you look at the documentation examples and other examples it should come as a default return and any condition that stops this from happening should be higher so it's better to switch the condition so if it's not role id of what we need then we'll return redirect back it's a bit more readable and more standard like laravel middleware and finally i would argue it's a personal opinion i wouldn't redirect the user back i will just show the page not authorized which is a board 403 no don't need to return like this so compare that to the original middleware role manager and this is our final middleware i think it's more readable and also we have three middlewares now but in fact they are so similar the only difference is what roles to check and we can transform that into one middleware let's do php artisan make middleware make middleware check roll for example right and let's copy that from rollmeister here into check roll like this and then this will be a parameter so we can add a parameter here for example string role then for example we have rows as a array of what roles should be checked for which name for example admin should be roll one meister should be one and three and manager should be one and two for more remember yeah something like that and then we have rows here and then if we register that middleware in the kernel file in app http kernel for example let's register it as check row then check roll class and then in here in the routes web we can do check roll meister like this and for this group it will be a middleware of check roll admin for here it will be role master role manager and stuff like that but instead of three files now we have check roll oh wait i've left one more thing so it should be rows roll like this probably we need to check that rolls roll exists in this array but if you are calling the middleware only from your code probably it's relatively safe or we can do something like roll ids equals roles role or if it doesn't exist for example this and then have role ids here okay enough of the middlewares now let's take a look at migrations and i've noticed one thing which is pretty common to junior developers is not creating foreign keys so user's role id is a field in users table which is just integer so it's one two or three and then rows table actually exists it was created as the last migration but there's no foreign key in the database level from users to rows and i assume it's because it was hard to refactor so if you have already the field how do you change that when the table is created later so one way to do that is create a separate migration file so for example php artisan make migration add role id to users table and current role id is camel case so in users we have role id a standard way to use foreign keys is underscore id so we have the old field and we will create a new one so role id here we will do table foreign id it's a syntax that started with laravel 7 role id constrained which means it will create a foreign key on a database level and will ensure that if some role is deleted then if there is at least one user with that role the delete operation will be restricted and fail so foreign keys on a database level will allow you to protect the data from accidentally deleting or updating something and now let's take a look at the controller code and make it better so in routes web i've chosen a controller of material controller class and what do we see here we have construct with middleware of rollmeister and this is not needed here anymore if you have the middleware in the web routes then you don't need to have middleware in the controller next what do we have index which is probably a list of materials and then material list which is also a list of materials and visually when i look at all those conditions and code the only difference i see is just different view so i would probably transform that into one method index with additional parameter of something like view equals one by default or something but let's not do that inside of this video it would take too long let's focus on transforming this one into a better code so what advice can i give here first small details but pretty important if you have a list of something please call it as a plural variable so material types like this model name should be singular but material types variable should be plural and it actually is plural when passing to the view but then we should try to be consistent in all the variable names now also i see that the view part this line is identical in if-else statements so in order not to repeat that line we show them afterwards so we return that view anyway the only difference is what materials do we pass here okay so that's better now if there's no filter there's material all but if there is a filter we filter them right so two things here first thing this query is really really inefficient so you should not make filters after you take all the data imagine if your table has like tens of thousands of rows so you get all of them first which downloads huge amount of data and then you filter them instead of that you should filter them on mysql level and take exactly what we need so where should be before all and all becomes get so all the condition and filtering should happen before taking the data so that's one and also there are a few ways how we can get rid of that if else and i will show you two ways so first we can use ternary operator and do materials equals if that condition like this then we do material all let's do it in a new line otherwise we do this condition so like this and then we comment this out a bit shorter and maybe more readable but that's a personal preference maybe if else is more readable for some of you but also there's eloquent when method that allows you to run everything in one query so materials equals material when some condition and then get that condition is if material type is zero then we do callback function on query like this and then we add query where type query where type we don't need gear here and we need to pass use material type filter like this probably space here let's close the sidebar so this is probably the shortest way or the most laravel way how i would run the same condition statement now if we scroll down through controller i have a general advice or tip work on your code style so a lot of inconsistencies so single quotes or double quotes space or no space so space between methods or not so for example if i click reformat code in my phpstorm let's see what it changed so formatted 21 lines it's quite a lot for a small controller so i can see that this developer should use something like code style psr 12 or just use phpstorm code format feature and not everything could be corrected by phpstorm so things like quotes should be done manually let's scroll down and see what happens next so this is a method where material is created let's focus on that one for a few minutes first according to laravel route resource standard it should be probably called store and not create because create is for the form but as i mentioned previously we won't refactor it this time but we will make this one a bit better so this one is validation and here we're checking whether the name code amount or type is null or empty in fact for that instead of this it should be request validate and then array of rules for the fields so name is required the rule is required and there are a lot of rules available in laravel and if we duplicate that line which is the code required amount required and type required and if that validation fails it would redirect back so it does the same thing and also it would redirect back with exact error messages and this statement doesn't return any messages so user wouldn't even know what happened so that's the validation laravel way or you can create a separate class which is form request so php artisan make request create material request for example and you can replace that request with create material request so variable stays the same but the class name is different and in that request we should cut and paste inside of that request we should change authorize to true by default and then this is the array of rules so we paste this here and then we don't need to validate anything in the controller so we put all the validation into request class and makes the controller shorter now to create a new material this is a valid way to do that but a shorter way would be material create and then you pass all the arrays so instead of doing request name code amount and type assigning manually you can do request all or even more secure way is request validated if you use form request classes so that contains all the array of the request and then you don't need to do all of these the only condition to that is that all those fields should be fillable in the model so you need to define fillable as array name code type and amount from what i remember something like this so four fields right so they should be fillable and then you can create them like this by passing the array and finally return redirect back is a personal opinion i wouldn't redirect back because you are not sure where that is coming from maybe in the future some developer will change that condition so you should redirect to specific route for example route of material order something material index whatever with success message so variable message equals success or something and then in the blade file of that route you would be able to use something like session message and show that message so that was just for demo purposes i will undo all of that now okay so here we shortened the controller method to create the material and final advice is about update form so there is update method and also there's update view and for all of that i advise to use route model binding which is part of route resource as well and it seems that developer tried to do that so you pass the model here but for that to work you need to structure the routes in a good way so use route resource here probably or pass the variable here as let's see update id so material it should be material so this name should be the same as this variable name that's why it didn't work so it could be something like you don't need id and you don't need this and material is resolved automatically by route model binding i'm not entirely sure that this change would work 100 i haven't tested it but it's the direction you should go if you want to use route model binding and again shorten your controllers and you can do the same thing here and instead of id you have material material and then you don't need find or fail here so then in routes web you should have material on both actually here and here like this so that's it short review but actually took quite a long time and i hope that review helps that person the author of the code also those of you who are junior or in the beginning of your laravel journey you got some tips and tricks on how to structure your code better and i think i will do more of these code reviews on this channel because i see that people kind of like that so i will have more material for this one from my students in my mentorship program so expect something like that more on this channel and if you want to support my efforts financially and kind of thank me for that you can check out one of three products that i have online which is laravel admin panel generator quick admin panel dot com one of my 14 courses at laravel daily.teachable.com or my latest release is livewire components at livewarekid.com see you guys in other videos

Channel: Laravel Daily

Views: 253,365

Keywords:

Id: sukS7QOBpK0

Length: 19min 57sec (1197 seconds)

Published: Sat Feb 06 2021