Junior Laravel Developer Code Review - Reviewing Routes, Migrations, Models & Controllers in Laravel

[Music] what's up developers is Zari here and welcome back to a new video where I will review the code of a larval Junior developer quick pause I'm currently working on a new udemy course where I'll dive into Davao databases in eloquent if you think that there is a topic that should most definitely be included feel free to drop it down below in the comment section if you would be interested in the course or want to support the channel make sure that you follow me on either Twitter Instagram or YouTube to be the first one to find out when the course gets released last week I tweeted out that I was looking for larval developers who would like to send their code so we could review them I got quite some responses and I decided to choose a project where I could add the most review quick note I've made a list of things that I would like to cover and I definitely would have done differently but that doesn't mean the way the user has done it is wrong I'm also not here to bash the user I'm just trying to help you guys out to prevent making common mistakes the mistakes that I've seen inside this project are mistakes I see quite often I also won't be running the project inside the browser and I won't cover the blade templates either I'm strictly going to focus on routing migrations middlewares and controllers so let's start off with the routing usually when I spin up a new application I start inside the web.php file to see the routes that have been defined so let's open the rights directory and the web.php file this mostly gives me a clear overview of what's Happening inside the application now right here it seems like the routes have been divided in into different files which is a good thing but the comments right here are kind of bothering me using comments is awesome don't get me wrong as long as it's useful right here you will find a common named art but we could all see that it's requiring the arts.php file so I don't really see the point of adding a comment Right Above It named art because we could all tell that authenticated routes are stored right here so what I would do is just simply delete it the final user makes sense in my opinion as well so let's delete that one as well the same goes for admin routes the only route buyer would understand a comment is a non-categorized routes but also there a single route kind of makes sense to me as well because I think that the author is referring to a couple of views that don't really belong anywhere so I will just delete it as well I have looked into a couple of routes before and I think that we can get the most out of the admin file so let's open the website directory and let's open the admin route now let me close off the sidebar for a moment I personally think that the comments inside the routes here are useless as well if we take a look at the first comment you'll see that it says articles admin index page and it's referring to the admin underscore index method inside the article controller so I will just delete these as well from the start I can see that almost all rights right here have a middleware of odd change to it which could be done a lot easier so let's define a new route right above of it which will be a grouped route we need to pass into parameters right here the first is an array while the second one is a callback function so that's a function and rather than chaining the middleware art every single time on all the routes we could simply say well add a key value pair right here where the middleware is equal to odds now we could even add the prefix of admin as well we could add a second key value pair inside our array where the prefix is equal to admin now since we have added our middleware inside the group we could delete the middleware on all routes now the first three routes that we have inside the prefix are okay so we could move them inside our grouped route but looking at all the other routes that we have I can already tell that something is wrong right here why are there so many extra routes for only the admin user we have a get request to the user's endpoint where it needs to call the admin underscore index method the resource routes that we have right here already has an index method inside of it so why not make that one a little bit more complex instead of defining a completely new route and view every single time so for the sake of this video we could simply remove all these routes above we could move the forward slash endpoint right here as well another point that is important to keep is that the endpoint should either be singular or plural now the standard encoding not only in laravel is adding plural endpoints so you have a endpoint right here named tutorial so tutorial should be renamed to tutorials so plural I've also taken a look at a tutorial that played a PHP file inside the admin subdirectory and all the static data that you have defined right there could easily be added inside a database so what I recommend is simply navigating to iterm performing the PHP artisan make controller named tutorial controller if it only has one method you could even make it invokable let's leave that aside for now hit enter our controller has been created which then allows us to define a new write inside the grouped route right below the last one where we could make a get request 2 forward slash tutorials it has a tutorial controller it has a class and we're going to use the index method that we have in the find and we're going to name it tutorials which allows us to delete this route right here now looking at the last available route that we have right here it looks like the content of the page is the same as the tutorials route right here that we just defined but simply has a different language because it has an underscore FR attached to it personally I would always recommend larva localization right here which is a convenient way to retrieve strings in various languages rather than defining a static page yourself I will add the link to localization in the description down below so please check that out and get rid of what you're doing right here now let's define the route for localization because we could do that inside our grouped route we could say route colon colon get the URI will be tutorials forward slash a route parameter of Locale I hope I'm pronouncing that right we could make a call to the tutorial controller colon colon class and let's say that we have a method of languages where we have a named route of tutorials. languages which then allows us to remove our route that we have right here now there's nothing wrong with the user's route and the rules right as well so let's copy these and we can paste them right inside of our grouped route which wraps up the routing part of this video now let's move on to the migrations of this project let's open our project close off the routes open the database directory migrations where you will find a couple migrations we have the users password reset file jobs access tokens a songs table categories rules and articles for above obviously makes sense to me the first thing that I'm noticing right here is that it's using a category underscore ID where the data type is a string and that's not all right because you're probably referring to a category stable it's also not an integer since a category ID is a foreign key it represents the ID on the categories table let's use our table object right below of it or we're going to use the foreign ID the foreign ID will be named category underscore ID we're going to chain a method named constraint which will be categories table and on delete we're gonna Cascade all the data now this looks way better also another quick note I personally find it cleaner to add foreign Keys either at the end of a migration or after the primary key usually I'll just put it right after the primary key so we could delete the string category ID that we have now the same goes for the user ID that we have defined right at the bottom so we could say table foreign ID on the column user underscore ID constraint to table users and on the leads we will Cascade all the data once again we can delete our table integer which is pretty weird because you used a string for a category ID but an integer for the user ID so be consistent with what you're doing if it's an ID it obviously should be a foreign ID but it's also an integer and not a string so let's delete it and let's move our foreign ID back to the top all right I'm also seeing a string of artists right here which doesn't seem right as well how do you want to get all songs of one specific artist the way you have it set up in your migration makes you write more and more code and you're giving the user the option to add the artist's name and you can't trust users adding the same exact name for every single artist so what I would recommend is navigating to the CLI creating a new model and Migration by setting PHP artisan make me a new model named artist and add a dash m flag to it for the migration navigate back to phpstorm open the latest migration so to create artist table let's say that we're only gonna add a new table string of name close it off then inside I create songs table we could create a new foreign ID for artists underscore ID constraint on the table artists and also on delete Cascade all the data this allows us to delete our table string of artists and we can then move our foreign key constraint up to the top the other thing is important and in my opinion is missing is the use of defining extra properties for your columns with the use of extra properties such as required nullable and unique you're making a stable database design that will prevent you from redundant data efficiently executes queries and improves database performance if your database design isn't right the code you will write inside your controller and models will also not be right now your entire migration is based on a song which obviously has a title that should be required and the slug as well the slug should be required and it should also be unique because you will run into errors when you have a slug that has multiple matches inside the database for preferences I would also put the slug right after the title because it's most likely based on the title now for the column names I would also recommend being a little bit more specific you are very specific when it comes to a song URL but not so much when it comes to a cover because it's simply a cover underscore image now I could cover all migrations but it will take a little bit much time and I think that with the code that we have improved right here you should be able to change up all the other migrations yourself as well if we open our let's say categories migration you'll see that it's using a name a color which is alright but it's using a string of user ID so this should be a foreign key constraint the rules is a right the Articles this should be rechanged to a well pretty much with any other migration has just a table ID you're using a string of user ID which should be a foreign key constraint as well you're having a string of title body the article image you're specific here and you defined an integer for the user ID which should be a foreign key constraint as well so we have created three foreign key constraints on the songs table which means that we need to define the relationships inside the song model as well because only having a foreign key constraint doesn't make it usable let's search for a song.php open the model I can see that you have to find the relationship between the user and the category but that doesn't make sense if you have not defined the foreign key constraint properly the only relationship that is missing is the artist you have added a column of artists so singular inside the song's migration where I'm assuming that one song can only have one artist unless you write tons of code to add them separately with a comma but in most cases there are songs with multiple artists so what I would do is basically create a new public function artist so plural now let me scroll down and we're going to return this keywords that has many relationship and we're going to pass in the artist model at the top I saw that you didn't had a fillable property for Mass assignment but the guarded property right here seems incomplete this could also be me but I think that if you want to specify which fields are not mass assignable you should add them inside the bracket as well if you want to add them all you should simply add single quotes asterisks now that was it for the model let's close off all files and we're going to look at a controller let's open the app directory HTTP controllers more I'm specifically going to have a look at the web subdirectory and the raw controller right here you will see that an if statement is being performed to check whether the user role ID is greater than two and we navigate down and open our database directory migrations and users table you will see that the users table has a column named row ID with a default of one but there is also a rule migration there are two things that I would recommend right here the first thing is using an enum over a rules migration since the data will always be stored inside your database and not inside your project so let's do that let's create a new directory inside our app directory let's name it enums where we are going to create a new enum named user rules enum.php now we need to create a file ourselves so let's start out with an opening PHP tag let's give it a namespace of app backslash enum we're going to add the enum keyword we're gonna name it user rules enum we're going to add three cases right here case number one will be user case number two is admin and finally we have a case called super underscore admin now let's close off our user roles enum and let's navigate back to our raw controller and I've seen this throughout your application while you're performing this check and even inside the rule controller you could see it in pretty much every function right the show edit update destroy and all of them now there are two solutions on how you could prevent this you could create a new middleware that will check the rule of a user or you could Define a new method inside your user model which you can access through the user object now let's do both and let's see the difference starting off with using a method inside our user model so let's open it and let's scroll to the bottom where we could create a new public function and let's name it is admin right here we're going to return either true or false based on the role of the user if it's an admin or super admin we're gonna return true otherwise it's false which can be done by saying return the odd user rule underscore ID if it's equal to the user rule enum and we're gonna get the admin case and if it's art user row ID is equal to the user rule enum and the case is the super underscore admin we forgot one thing right here because we need to chain the value of it which will be the integer you could add this method inside your raw controller as well and I've looked throughout this project and I've seen that it uses the if statement in a lot of places so not only the raw controller so I think that the user model is the best place for it what we could do then inside your controller pretty much redefining what you have done right here so let's say that we want to return we want to return the authenticated user and we want to get the is admin method if the return value is true return A View to let's say admin.rule.index and we're going to add with the width method the rules where the values are coming from the user rule enum and we're going to get all cases so let me outline this on the line below all right in case the return value is false we're going to abort it with a 403. which pretty much allows us to remove the entire if statements the second method is using a custom middleware so let's navigate to the CLI and let's perform the PHP artisan make me a new middleware and let's name it check user rule where we could navigate back to phpstorm open the middleware directory where you will see our check user rule class then right above our return statement we could basically create a new rules array where we have a user inside an array user rule enum the option is user and we're going to get the value then we have the admin the user rule the user option and the value comma we can get the user rules enum again where we could get the admin value finally we have the super underscore admin which has a value of user rule enum admin and we're going to get the value then inside our handle method we could perform a check to see whether the user has the right rule or Not by creating an if statement we're going to not check inside an array we're going to check if the authenticated rule ID of a user exists inside the rules so let's say odds a user rule underscore ID comma array rules if it passes we're going to abort it with a 403 we don't need a return statement now before you could use the middleware you need to register it inside the kernel.php file right at the bottom inside the right middleware where we could create a new key of check user rule make a call to the check user rule middleware class now the downside of using the middleware is that inside your admin.php file in your routes I'll let me open it our current route group has a middleware of odd so you can add multiple values right here obviously but that would mean that all routes will implement the check user Rule middleware and since the user of this project performs the check in multiple controllers but not in every method you could Define two new group routes where all the routes will be stored where the user is an admin so let's define it right above let's say route column column group it has a middleware which is equal to another array where we're going to pass in the alt and the check user rule whether it's an admin or not we're going to add a callback function so function parentheses curly braces where we're gonna move the rules resource right inside of it but whatever a rule is equal to admin we will only give them access to the index and show method which I have seen before because the index method and the where is it show method checks for ID number two now you could create another route as well well let's actually duplicate it where we will check for the super underscore admin Rule and we're going to give the user access to let's say the create method the store method edit method update method and the destroy method now we could even simplify our rule controller the index method what we have right here to return a view of admin.rule dot index with the rules which are coming from the user rules enum so we could delete the return statement that is checking whether a user is an admin or not our middleware will check inside the admin.php file what the rule is and it's giving access to the view the one thing that I'm noticing right here and I don't know the entire scope of the project is that you're returning a view of admin.roll.index meaning that you probably have created static pages for all rules this is something that you should prevent with the data where you could simply return back one single view for every single rule but the data will change based on the rule now if we scroll down you will see that the create method performs the same check which we could remove where we could simply return the view we could delete the entire if statement we also don't need to abort because that will happen right inside of the middleware the store method uses the same exact check right here which you can delete so let's do that first we don't need the else statement all right now personally I'm a fan of separating validations from the controller since the validation needs to be done inside a request so let's navigate to the CLI perform the PHP artisan make me a new request named store you user request let's navigate back to phpstorm let's open the give me a moment the request subdirectory store user request where we need to change the return value of the authorized method to true so that the user is authorized to make this request where inside the rules method we could simply Define the rule where the name is required if we then navigate back to our roll controller we could remove the entire validation that we have because we're not going to use the request class but we're going to use the store user request we do need to validate it by saying well give me the request validate it now after the validation we're ready to create a new rule but instead of using the All method we're gonna use the only method and we're only going to request the name the reason why I don't want to use the all method is because you can't trust users and you don't know what's coming in let's redefine our redirect as well by saying return a redirect to the routes of rules.index but we're going to add a message to it because you obviously need to tell a user whether the creation of a new role was successful or not so let's say message and the value is role has been created so let's delete the entire route that we have the show methods could be redefined as well so we don't need to check and we also don't need to abort all right the edit method could be changed as well we don't need to check and the else statement all right then we have the update method where I recommend using a new custom request but since it's using the same rule as a store row request we could simply use that instead of creating a complete new class so let's replace the request with the store user request and if you have time just rename it to store or update user request we don't need the if statement and the request validation we don't need the else statement we do need to validate our incoming data by saying request validate it we're once again not going to update all values but only the name we will return a redirect but with a message once again saying rule has been updated now let me outline it now let's have a look at the last method which is the destroy method we once again don't need a if statement and this looks alright so we could leave it as it is so let's only add a width message right here named rule has been deleted now keep in mind that it is debatable if using an enum is the best solution right here since you can create roles through an interface I still think it is because it eliminates all the static numbers when checking for a raw IDE I also think that we cleaned up the controller quite a bit we don't have that much code for a controller this application has tons of files and I can't cover them all but I want to point out some important things I see Junior developers doing wrong the first thing that is bothering me is inside the controllers folder where he has a tutorial controller which is completely empty if you don't use a controller model migration middleware or whatever just delete it we then open the art subdirectory inside the controllers directory you will see that it consists of an Aus controller file now I don't see the point of adding a subdirectory right here of art if the name of your controller is the same if you name your controller the same and put all the authentication methods right inside one single file what I recommend doing is simply making a login controller and a register controller just follow the larva obese controller naming conventions right here also another recommendation is to use larval its method name in convention inside the Arts controller you have a login method which actually shouldn't be log in because the login is actually the auth method which will check the credentials of a user and sees whether a user can log in or not well down below you have a log out method which is alright you have a register method which actually should be the new method of you where you validate the incoming data and create a new user and even the return view can be put inside the register function now this was it for this video where we covered the migration's routing model and controller of the code that has been sent in from a junior Largo developer if you want to see more content like this let me know in the description down below so I could do more code reviews if you do like my content and you want to see more leave this video a thumbs up and if you're new to this Channel please hit that subscribe button

Channel: Code With Dary

Views: 4,814

Keywords: laravel junior developer code review, Junior Laravel Developer Code Review, laravel routes, laravel reviewing routes, Controllers in Laravel, laravel migration, laravel model and migration, laravel model and controller, laravel Middleware for the User Roles, laravel Middleware with Parameters, laravel junior developer mistakes, junior laravel developer, Tips for laravel developer, tips for junior developer, laravel code review, reviewing Laravel code, laravel junior code review

Id: zCoFpViOHts

Length: 26min 29sec (1589 seconds)

Published: Mon Mar 06 2023