Junior Code Review: Cleaning Up Laravel CRUD

hello guys this time let's have another code review of a junior developer and this is the context i'm showing you the actual email actually blurring the contact details of that person it's a project of a junior developer first solar project after completing six months full stack developer course and it took two months to create it and this time for the review i will do it a bit differently i haven't looked at the repository at all so for other code reviews i prepared quite a lot i've made a list of things that i want to talk about and this time let's start with installation of the project and in almost live mode i will go through all the basic things like routing controllers and all of that and let's see what we can find so i started with the installation and i've done composer install and then my great fresh seed and here's the first era that i found so the project is not installed because the name of some migration is already in use and here i have a first tip for you so there is a migration called update static pages table to add a link to some static page database table right and then there's another update static page table on a different date so you can see timestamp is different here which adds another field so tip for you is to name your migrations in a way that they would be unique i'm actually not sure how it worked for the developer because it should have thrown errors anyway but let's try to fix it to fix that we need to rename one of those migrations so let's try update static page table icon for example and then we need to rename the actual file name so static page table refactoring name table underscore icon so those names should be identical and then they should work let's try it out again migrate for seed and now it's successful so this is a tip for you you can manually rename the migrations but with the rule that the name of the migration with underscore should be the same as class name in camelcase next we load the home page and we see this and i already see that this code review may turn into let's make it work review with lesson how to provide your project for someone else to review so obviously some link is missing of non-object if we go to our home page blade which is this one i found that there's settings link and settings title and those settings come from the controller front-end controller it's actually a laravel 7 project so the syntax of routes is the old one front-end controller so there is a settings find one and it should be lowercase actually but then it implies that the settings already exist in the database but there are no database seeds here if i go to seeds in database heater i see only roles user and countries but in the settings database table there's nothing so if you give your project to someone else to your teammates or even to yourself to install on a different server or if you buy a new computer or something like that you need to provide the c's enough that at least page would load but i assume we need to create those settings in some kind of admin area so let's go to our web php and let's see how we can log in okay we have author out so it probably is slash login okay it works interesting design let's see how we can log in user seeder is just faker name and email so we don't have that user as admin or do we have it not sure let's try to log in with that email and password is what password oh temp one two three four five okay let's log in great we're in now probably we should go to the settings and it's empty is it a form no it is just a set of fields and we need to go to new settings right then probably fill in the form i will use fake filler chrome extension to do that and let's try to save it create settings okay so latitude and long it should probably should be in some different format so a lot longitude let's actually delete it maybe they're optional i hope create settings cannot be no okay try again let's add point one and point one somewhere in the coordinates in europe oh successfully created great now let's try to launch the homepage we go to our homepage and yeah we have something of course it's all testing data but it at least shows something doesn't load google maps that's fine but that's some kind of a home page without an image because i haven't uploaded the logo i think but it kind of works next let's see what's on that home page from the controller point of view so in front-end controller we already saw that we query all of those things that probably are needed for the home page and then we formulate all the big data and do we need that let's try to format it a bit so for more readability like this and i think we can use something called compact here so here those variables are the same as the keys and probably we could use compact or at least we can probably get rid of those variables so instead of initializing the variables and use them only locally why don't we do something like this so settings all here and then one by one slider all like this projects all actually i've missed it so projects all they're not even in the same order so services all should be in the services here then settings should be this one then partners here static page and why static page isn't singular everything else is in plural actually not everything category is as well so naming things again i'm repeating that every time with almost every code review please make it clear so variable names are okay but is it one category or is it multiple categories if it's all then it's probably a list same here static page static pages it should be static pages and categories something like that so we made our controller shorter without temporary variables like this and if we go to our home page let's search for category no not that one we don't even see the category here probably it isn't some kind of include not even sure so probably some of those variables aren't even used on the home page i may be missing something but static pages they are used here so static pages like this so another tip don't provide the variables that you won't use next let's go through all the other methods or some of the other methods in front and controller and let's see what we have here project slug i also see a lot of junior developers do something like this that sign is not necessary so this would work you don't need to provide equal again settings fine lowercase that's a small detail and again setting up all the variable with this so i won't repeat the same thing i probably wouldn't set up a variable although actually in this case it makes sense because it is used in other case but then wait a minute we're acquiring project and we are acquiring galleries of that project is there a has many relationship there is a has menu relationship so why don't we do something like projects with gallery or galleries gallery it should be probably galleries because it's many so with gallery like this and then we don't need this one and we can delete this one as well and then we can access that gallery in project blade let's open projects blade for each gallery for each project gallery something like this okay moving on services same thing it should be less code and has many relationship projects order by id descending is a shorter way latest paginate and i see settings everywhere so couldn't we move it somewhere so settings here let's check all the methods settings here settings here so it should be some kind of global variable you could potentially do something like public function construct in the same controller and then do this settings equals setting find one and then it should be private settings so if it's used just in this controller only on the front end this makes sense and then you can change that to settings or this settings actually like this or if it's the same variable used in multiple controllers then you should do something like view composer or view share i will link in the description below the link of laravel documentation how to do that for multiple controllers now let's take a look at that route group for logged in user and i see that it's checked for user admin i'm not sure if it's needed here because there are two groups that are public routes and then there are admin routes so auth i think it's enough if i'm logged in i'm an admin in this case maybe it was in plans to have more groups and then that middleware makes sense anyway i would delete that and also i would delete the web because i think it's a default middleware which works for all the web requests so we don't even need that array middleware auth like this then let's go to home controller but then i think it should be renamed to something like dashboard controller because we have already the route home which is front-end controller and then the home controller actually means dashboard so it should be something like dashboard here like this but let's roll it back and let's see what's inside in the home controller it's just a static page so nothing to find here but empty lines and code formatting i will not repeat the same thing in every junior code review but please read about psr standard and how to format the code so next user controller what do we have inside of that construct user middleware no we don't need that because it's already in the routes web here and then in index we have users again we need to shorten the code so let's do this and we don't need that user's variable at all then we need to create a user so we have rows here and we have users it doesn't really make much sense to have users in the create unless there's a parent form of some kind let's open user create and let's search for users nothing found so probably my guess is that we don't need that users and again the lesson is do not initialize variables that you don't actually need or use let's go to our users table add a user and it works then i saw briefly in the user create form is used like url user i would vote that you would use route names so route user store i think the name it is refresh it still works and then instead of csrf field you can do it a bit shorter refresh we didn't break anything cool then in the store again empty lines why do you need that validator make and then if validator fails this can be shorter i'm a big fan of form requests and you can see that in my other reviews but if you want to initialize the validator manually you can do it simple this validate and then you put in all the requests like this and then that validator fails is done automatically so that validator will automatically redirect back with errors and with input so you don't need to do that manually then we create the user and there is some kind of variable which isn't used so we don't need that and that's it okay then show empty methods don't leave empty methods if you don't use them edit the user this looks fine update it could be done in a better way so look for my other code reviews for update methods but basically instead of request all it should be request validated and you could use form request for that and then destroy user all good okay let's take a look at other controllers for example let's take a look at static page controller and i'm assuming it should be the same thing or similar things everywhere yep most of the controllers probably are just cruds with some more logic like doing the slug this is cool again assigning the variables and doing the save instead of that you can do something like static page create like this and then provide the variable like request validated if you use form requests i've performed that a few times in other junior code reviews so please watch those okay and all of the others are probably cruds so doesn't make much sense for me to review again actually interesting thing image store what is image store this thing i like that image store is a separate function in the controller but not sure if it should be public or private if it's used only from the same controller and then make the folder for original thumbnail and medium also have a separate junior code review for the file upload so you can watch that i should probably advise to use sparty media library package for this which includes thumbnails and medium sizes and all of that and it makes this all huge portion of code is a few lines in spitey media library so i guess that's it for this code review all the other controllers are more or less cruds also file upload pretty similar oh image store is different from controller to controller so that should be totally spotty media library it should not be image stored in every controller okay that's all that i have time for now in this review let's wrap up i will link in the description below to other code reviews that i've mentioned in this video so to avoid repeating myself i probably should stop reviewing all the code from end to end and will start shooting videos on a separate topic or separate mistakes that i've noticed in the repositories of people who sent me the repositories but i will still have a few code reviews on this channel planned in upcoming weeks so stay tuned subscribe to the channel and see you guys in other videos

Channel: Laravel Daily

Views: 35,631

Keywords:

Id: 5FD9CxBbs_A

Length: 14min 53sec (893 seconds)

Published: Tue Mar 16 2021