today we're going to do another junior code review
the code review is about a tiny project a developer did as part of an interview
code challenge unfortunately this particular developer did not get the job
and there were a few reasons why we'll start off by covering the minor aspects as to why he quote
unquote failed this coding challenge and then we'll move on to the two bigger reasons as to why
hey guys i'm dan and this is big suffix and i'm here to hopefully help you improve your code by
doing code reviews and pointing out common mistakes junior coders tend to make if you
like these kinds of videos and you would like to see more please hit the subscribe button and
also turn on the notifications so you don't miss out when i upload more content like this okay so
let's jump straight in and look at the project the project is a simple application that shows
a welcome page with a select drop down with a bunch of cities now if you click on one of the
cities and you click view weather it'll show a three-day forecast of that city's current weather
okay and now if we look at the code we can go into the web the php routes file now the first
thing you can see here is naming conventions are not properly followed okay so if we look at what
this show where the index method actually does is if we go to the weather controller
you'll see that it only returns a view so there's two ways we can actually fix this the first is we
can rename this method to index only and then in here we call the index method now this is a better
way of doing it in a sense of naming conventions but seeing that this method only returns a view
this method is basically redundant so we can remove this method and then instead instead of
calling a get route or the get method on the route we can just call the view and we don't call
the controller method anymore we just call the view and now if we go back to the web and we
select another city and run it again we'll see it still works okay so another thing we can quickly
look at before we move on is cleaning up some code now the developer told me that this api
or get api forecast method is an unused method if you have any unused methods or any kind of unused
code remove them from the code base it just makes it easier to read and the code reviewer doesn't
need to spend a lot of time trying to figure out what methods do uh when they are actually
unused okay so the next thing we can look at to improving this code is um renaming this method
now this method get weather forecast receives the city from the front end and then it checks
or it calls the api to get the three-day forecast for that city and then returns the results now
naming this method could be confusing because from the the routes file it is called via a post method
and within here we do we do some kind of validation but we're not storing any information
we're just returning it so what do you call it you call it an index method a store method a show
an edit that's a good question okay so i would say in this in this case there's only ever going
to be one method in this controller so we can simply just call this method invoke which means
whenever this controller class gets called or gets initialized it will automatically just
invoke this method or simply just execute the code within this method now back in our routes file
what we're going to do is we're not going to call the get weather forecast method anymore because it
doesn't exist we're only going to basically call the class or the controller by its name and now
if we go back to the web we select another city and we view and there we go we still get the same
results okay so back in our invoke method within our controller you can see the weather class is
being nude up and then assigned to a variable this is a common mistake junior developers tend to make
because we can get rid of this line of code and rely on laravel's dependency injection to inject
that class for us and again if we go back to the web we pick a city view the weather everything
still works fine now again back in our invoke method within our controller we can see that
the validation is being done inside the method or inside the controller now a better way for doing
this validation is by creating a dedicated form request and we do this by calling php artisan make
request and then we call that just weather request now what we can do is we can take our
validation rules and we can get into our weather request okay and then we can just basically
clean up this code and rely on the type hinting the return type and by default we
need to change this to true because we're not doing any authorization within this request
and we can just return an array and then what we do is just copy our rule inside the rules method
now back in our controller we can now ask laravel to inject our weather request rather than the
default request and we can remove this line of code and now if we go back to our web pick a city
and everything still works fine now as you can see we've reduced the size of our controller by
a significant amount there's still a few things we can do within the weather service class which
is the class the developer used in order to fetch the results from the api but we're not going to do
it in this review otherwise it will just take too long now this brings us to the first big reason
why this developer did not get this job and the reason is testing there's absolutely no
tests written to test the functionality of this application and testing is such a big part of the
development process these days that if you write tests within your interview or your code challenge
it'll just improve your chances of successfully passing the code challenge
just a lot more okay so let's go ahead and write a couple of tests just to show what this developer
could have done to increase his chances okay let's create a test by calling php artisan
make test and we'll call it uh we'll just call it weather test okay now within
our weather test we can simply rename our default test so it should display
the home page all right and then what we're going to do is we're going to yep given keep the
response that we're going to visit the home page right we can just call it see if our named routes
work by calling home and then what we're gonna do is we'll just chain this on we can then insert
okay so we don't need to assert the status 200 we just saw that it's okay because okay is 200 and
we can just get that take that off and if we're in php unit
three test run three test passed okay and that is because we've got the example test
which we can basically remove and on the unit we've got use this as well that we can remove
now if on php unit it should say one test one assertion okay so the next thing we can do is to
assert that we actually see something on the home page because this assertion only checks whether
there was any errors in the response or not so what we can do is just chain on another assertion
we can say assert c and then what we want to see is on the home page we want to see welcome to
weather wizard so back in our test we can just say welcome to weather wizard and now if we run php
node again and we can see everything passes okay so another test we can add is basically to test
whether our validation works so we can say it should return an error
when validation fails and now we can do our post because we need to post to that endpoint
and our right name would be home and
then we're not going to pass we're not going to pass any information like the city name
to the endpoint because we're expecting it to return an error so we're expecting it to return
a redirect error because it's going to redirect back to the same page and then we want to assert
that the session has a key called errors okay so if we run php unit again and now we can see these
assertions were correct okay and then for this example our last test we can do is basically it
should fetch the weather successfully and then we're going to post our data
to our route home and then we're going to pass in some information
called city and we're going to say we want the the weather for london
and then next we want to assert
that the response was okay got a 200 response and then we also want to see something on the screen
that says that we've got the the weather for london so we can say assert c
so it's c and then what we want to see is the weather for london
so we can just say view the weather for london and then we just copy this because we want to
see that on the screen after you run our test we can just paste it in there we want to see
the weather for london all right now if we run php unit again all our tests are passing now
for a simple application like this running writing these three tests could have taken this developer
maybe 20 25 minutes or half an hour even if he didn't know anything about testing just to
read up or do research about testing but it would have increased his chances of successfully
completing the scope challenge a lot more so pro tip guys please do testing write some tests in
your code interviews or your code challenges it's definitely going to increase your chances
a lot of successfully completing your code challenge now this brings us to the last
reason the developer did not get this position and that is because the code challenge asked for
a front end to be developed in react and tailwind instead the developer used blade and bootstrap to
develop the front end so this means that the developer did not complete the requirements
as specified in the code challenge so my tip for this is if the code challenge requires you to use
a specific technology and if you've got the time to do it please do some research and try and use
the technology they specified it doesn't really matter then if your code is not great at least the
interviewer can then see that you've tried to use the technology they want you to use and not just
that the interviewer can then see you are willing to change and willing to learn a new technology
in order to improve yourself thank you so much for watching guys i really appreciate it
and if you like this video or if you found it useful in any way please give it a like and also
if you haven't subscribed yet please do it now and remember to turn on the notifications so you
don't miss out when i upload more content like this until the next episode guys see you later