Laravel Junior Code Review: 12 Tips on Everything

hello guys i recently received this email asking for a junior code review and i realized for quite a long time i haven't done any junior code reviews and it was pretty popular a few months ago so if you go to my laravel daily youtube channel among popular uploads number two is junior code review with astonishing 137k views but for a while i haven't done any code reviews i was focused on other topics and let's make an exception here and make a review for this person for the repository of recipe management it's a pretty simple code from junior developer and i will have quite a few tips how to improve that first the installation of the code and first rule if you give the code to anyone on your team or publicly it should work the installation should work so i cloned the repository i've run composer install it works but if i run php autos on my grade fresh because i have something in a database seed i have an error and now i have to dig deeper why is it not working some foreign key constraint and stuff like that i almost feel like kind of a disrespect from that person like they're giving me the code and then it's my problem to make it work but okay i've spent the time and i found that there's a folder called modify migration for some reason that has two migrations for some reason in a specified folder although they should be in the main folder so let's drag them here refactor and then likes here into the migration and let's delete the modify migration table so that was step one why it failed and then there will be step two so for now let's refresh seed it's still an error but on a different migration so the commons migration and the reason for that error is the foreign key field integer versus big integer so see in the create comments table which actually throws an error there's a foreign key for recipe id with integer field and recipe id comes from recipes table where the id of recipes is table id and by default it is big integer not integer so from that mismatch we have a migration error and by the way this error is not really clear or readable and it doesn't say where to fix what it doesn't say anything about integer or big integer it's just general sql error cannot add foreign key but the fix is to have big integer instead of integer also for other foreign keys as well for user id and also in other migrations i saw the same thing so big integer big integer and unlikes also big integer big integer okay now if we refresh again now it is successful so step number one is installation of the repository which wasn't that smooth but i made it work okay so now we launched the project visually successfully and probably we can log in but what are the credentials probably they should be in some kind of readme file nope on github there's no readme file that's another feeling of kind of a disrespect maybe i'm phrasing it in a wrong way i don't want to disrespect that person who wrote the code but generally if you give the code to someone else please make sure that they don't have to spend extra time to even launch your code or figure out how to log in or something like that put in the default credentials or default users or something like that and to be honest there is a seed for database heaters user seeds so there are 10 users seeded automatically which is ok but what are their passwords how do i log in with them if we go to factories for example there is a user factory and what is the password okay it is password so probably i need to go to the database get the email of some person and then try to log in with password okay i'm in but why is it not in readme or anywhere else and even then new recipe for example i want to add a new recipe and i want to select the category there are no categories how do i add the category apparently it can be done only by admin user but admin user isn't seeded there is no admin user here so see how many problems i encounter and that's what i wanted to emphasize not because the code is bad but even the setup for new employees for new teammates for new reviewers is pretty complicated so to solve that i need to again one of the possible actions i need to go to the database setup is admin one for my own user and then if i refresh i get the menu categories and now i can finally start working with a project the project itself is a few cruds so create category for example something something we submit then we go to the home page we can create a new recipe i will use fake filler chrome extension to add something here and there we submit and there is a recipe then we can read more about that recipe add a comment and every comment may have likes or dislikes so that is the instance of the project that's all you need to know visually now let's dive into the code first thing i always look at in a new project it's routes file so routes web and what do we have here let's close the sidebar for now it is using an older version of the syntax of the controllers which is with ad symbol which was the original from laravel 7. in laravel 8 it changed so to make it work for those of you who maybe don't know in the route service provider there's a setting called namespace so if you have that uncommented then you can use that syntax but to be honest i already got used over the time that laravel 8 is up i got used to that syntax of user controller and show user comments like this or in fact user controller class it should be here so our user controller class like this it looks longer on the surface but then it is clickable and i can immediately land in the controller when i need that so i would personally vote for this syntax but that's a personal preference especially if you started before level 8 on laravel 7 or earlier you may be used to that syntax and that is ok it is working but what i don't like immediately in this case is inconsistency in method names so show user comments is the method in camelcase and change password update password is in the snake case why if you want the correct answer of what it should be according to psr standards it should be camel case and i will link the documentation of psr standards in the description of this video but either way this is not bad the bad is the inconsistency so if you have both then code becomes messy so even if you don't follow psr standard which i don't recommend but if you do for whatever reason at least stay consistent next if we scroll down there's get before resources and this review is not only about bad things i also will say good things so this is a typical thing which i see other people's make mistake in so if they have route resource and then some separate slash recipes something so if you put these underneath the resource it will throw an error or in fact it won't match those because it will conflict with recipes show so a good practice is to put any additional resources additional routes before the route resource so that's a good part then let's scroll down and this part is pretty questionable in my opinion so group for route resource accept show and then out of that group the same controller with only show so i think the logic here is to have category controller for admin to manage the categories and then only the show method would be available to other users not admin and this works but i would generally separate the controllers so if you have a route for general user to show something then it should be user category controller if there's admin functionality it should be in admin category controller that's again my personal preference and personal opinion but it should be i think in some kind of subfolder with namespace like user category controller and here you would have admin category controller and then having route resource with just only method i don't think that's a typical way so i would do just get categories category here probably and then category controller show with name of whatever name it needs to be so it's not actually route resource it's single route get and then final thing in this route that i've noticed likes and unlikes for the comments and generally if you follow the comment like and unlike it creates like stable and unlike stable separately again that works but i would vote for one table of for example likes and then one of the field will be boolean or tiny integer of one or minus one something like that and then you would be able to calculate likes or unlikes from one database table with the same single structure and there are also packages for that for example i have googled and found laravel love which was introduced back in 2018 and i think there were at least a few more packages which allow you to just like like buy likes count and provide you basically with full functionality of likes and unlikes on whatever model you choose so i would vote for that one so that's probably it about routes what i do like though is the separation of groups off routes it's not even route group maybe it's possible to introduce route groups but not necessarily what i do like is the visual separation with the comments so users recipes admins categories and stuff like that that is a good thing okay next feature i want to show is the route model binding with slugs as route key so for example if you go to recipe the url is slash recipes slash slug and i do like how it is implemented here so there is route resource for recipes and if you go to recipe or controller again i cannot click it directly because the routes are not in laravel 8 style but i can go through that controller and go to show recipe recipe and in the recipe model there's get route key name so for those of you who don't know you can change the slug of route model binding so there is no find or fail here it happens automatically with route model binding here and that slug is set also in the model in the set title attribute that is an interesting case i don't really advise to do that and i'm not a fan of doing that is using two attributes inside of one mutator attribute so i think that laravel feature was created to set one attribute so transform the title to first letter of capital case and that's good but the slack should be set automatically in somewhere different place because it's not a set slug attribute and i have a separate video pretty recent on how to set the default value for some invisible field like slack or user id in that video case and i will link that video in the description below but in this case it works the slug is set automatically here and then is serving as route model binding key and finally let's discuss one of the controllers probably the main controller where the action is happening in recipe controller and first thing i want to mention here is i personally don't like the middlewares in the controller while it works and it is possible but personally i prefer to know the middlewares in the routes so here it should be not only prefix for admin but also which middleware is used and here for all the non-public routes it should be probably these ones all grouped with middleware of auth instead of having that here in the constructor of controller it's mostly because it becomes a hidden logic so in the routes i don't see which routes are public or for admin maybe i can guess it from the prefix but then i need to check in two different or even three different places who has access to what in this project so middleware in routes is my personal preference then in the recipe controller we have this recipe new recipe why is it needed let's search for this recipe here and in the show of the recipe we have format ingredients and what is it returning for go to format ingredients it is returning ingredients okay it is comma separated ingredients so it's transforming that into array okay but that's probably not the way how you should do that format ingredients should be an attribute again if you use in the recipe set title attribute in a similar way you should do public function set for example ingredients array as a separate attribute it shouldn't be the same as ingredients attribute and then return that so return this ingredient sorry return it's not in so it's this ingredients this something like that and then whenever you need that you use ingredients underscore array as a property of that recipe model so you don't need to do this and you don't need to define a private or protected property with new recipe that is kind of an over engineering okay in the index it all looks cool getting the recipes getting the categories nothing really i can comment here create form is also cool with the store there's some logic to add an image and this is i really like auth user recipes create it's a short way so to not set user id it uses has many relationship to all fuse recipes cool i like that probably this one may be transformed into don't repeat yourself logic of for example recipe this so we create the recipe anyway whether the image exists or not so that is the condition so we create the recipe oh sorry auth and then if the recipe has an image then we do recipe update because we do have already the recipe record with just an image something like that so then the controller becomes shorter and it doesn't repeat the same code i hope i didn't break anything but you see the logic there was a duplicated code and i extracted that before the if statement and that's probably all i can say about this repository everything useful everything else is more like details here and there which are personal preference i hope that code review was useful for the person themselves and for you guys and if you want more junior code reviews i probably will get back to those because they are really popular on this channel so you can send me your code to review i don't promise anything yet it will depend on how much free time i have and how your code is actually useful to others but you can send me your code to review i may choose your code email is powerless laravel daily.com so send me the link to that repository tell me your story what is the project and also invite me to the github repository username is populouscorp and see you guys in other review videos on this channel subscribe to get daily videos

Channel: Laravel Daily

Views: 47,288

Keywords:

Id: twPM-6X0pMI

Length: 15min 29sec (929 seconds)

Published: Thu Sep 16 2021