I have 20+ years of experience. I met every type of developer you could ever meet. Once I worked with a senior developer who didn't believe in indentation. He wrote code on few lines and even mocked me when I indented his code every time I had to correct one of his bug. On another occasion, another colleague, I had to search for a bug in one of his controllers, that was more than 1000 lines long. In the past I made mistakes, of course, learning is a "long" process, but I always tried to do my best to write clean code. Sometimes other people just don't seem to care.
Lol, I can feel it... In my freelance journey, I had worked and still working with a team of devs, where most guys really don't care about the pain of others. It mostly happens when the most senior one ignores it.
json_decode($response->body()) can be refactored into $response->json() Nice demonstration of how nested if-elses can be improved drastically! I hope the author would see this video and learn from it.
I new versions of laravel? I think that Guzzle remove the json() method because it was not in the specs of PSR, but i am not sure if the laravel facade Http also remove the method
You can even take some nested object in the JSON like this: $json = $response->json('rows.0.elements.0') and it will return null if that object doesn't exist
Seniors sometimes write code which smells. But before making any conclusions I think there should be some clarification questions regarding this code, for example: - Does the code work? - How well does it work? - Maybe there was some deadlines to write this code? - Is release was more important than quality? You have to consider this and probably much more questions together. So "Keep Calm and continue refactoring your codebase to better code and design"
That is true, but if You are 5 levels deep in nested if/else then You should consider that something is wrong :) but as You mentioned there are multiple reasons for this code to exist
One of the point of "seniority" is to know how to write clean code "already". I can't imagine that monster CEO who was standing behind, yelling "FASTER YOU BASTARD!!!!" and that "senior" suddenly forgot early returns concept. More logical for me is sabotage before quitting 😂
I believe the only aspect I would approach differently is implementing a service class specifically for the Google Service. However, I must acknowledge that your version of the code is more streamlined and organized.
It should definitely reside within a service class, and extracting the code into the service is not in collision with having the streamlined and organized code.
I think the foreach could be changed with an in_array, that also can be early returned. Then at the end only return what you need so the logic is fully complete.
i don't think it would work as in_array will search for the whole destination value..but in this case you just want a part of the destination so strpos/str_contains is just fine
I would have done the same, except extracted only the elements since other parts of the response aren't used: $json = $response->json('rows.0.elements.0'); if (empty($json)) { throw... }
I would add one more change: use a different message in the last exception. The second exception (after the refactor) is when you don't get well-formed data in the response. That's different from not getting any UK destinations (the third exception). If all you have is an error in a log, it's nicer if you get a message like "no UK destination received", then you know where it went wrong.
You could have moved the foreach into the IF block above with the 3 conditions and made them positive checks instead. So if all the checks passed, the loop would run and possibly return. Else, the Exception would be thrown at the very end. Just a minor style change and removes the duplicated Exception
I am not a senior developer, but I have had the chance of working on projects developed by seniors, which gave me such a great opportunity of refactoring, especially making controllers
Thanks! Never found (or looked up) this in the docs, but it is! BTW you can only check the 2nd variable, as isset returns false, if the rows property or an index on it is not set, it does not throws exception. (It is also new for me, that isset is like nullsafe at any part, but tested in PHP sandbox now back to 5.2.0, it worked - never tried this before (~13 years in PHP development...))
@bubblelong4759 the commenter is talking about the person who wrote the code being an imposter(fake) as a senior, accusing them of not being as competent as a senior developer would be. the commenter has not made a comment on laravel daily
I would use Laravel throw_if() function instead of the IF statements to make the code even more elegant, readable and somewhat with less lines letting it all in Laravel house of tools. Thank you for another Great Video!
Excellent video. I always tell my web team "write your code under the assumption that the dev who inherits it is a homicidal maniac who knows where you live". 👏
Nice, it removes a lot of mental burden. One more thing I would is the foreach section I would extract it in a separate method let's call it UKDestination() and the I would move the last exception throw above, $UKDestination = $this->UKDestination($parameters); if ( empty($UKDestination)) { throw \Exception(''postal code not valid') } return $UKDestination Maybe it's just me but I prefer the happy path to be always at the bottom otherwise it messes my brain up . Thanks!
Yeah, good suggestion! I was actually thinking to use some collection operation instead of a longer foreach loop. But separate method also makes sense!
Wow, I'm amazed by the level of the comments here. Makes me feel good that I worked in a few places where developers were kind and cared a lot for helping and improving and trying to understand the situations we're at sometimes. The post itself mentions they received an application. That is kinda open, isn't that? If that's an application for a dev role there can be a looot of stuff underneath it. What was the deadline? Was it live coding? There are a lot of developers who crumble under the pressure of having someone on their shoulders, which is the same as live coding. Same goes for a very short deadline. How much code was expected in the given timeframe? There are a lot of job applications that give you a deadline that you are not expected to meet without leaving stuff behind, I've had a few in the past. Also the person exposing it was posting it to feel superior and get engagement from people that would also want that - which is pretty obvious given, you know, it's from twitter. Judging other people's skill for some random code you see without context isn't useful for anyone is not healthy at all.
one of my pet peeves is initializing values in the return statement. every time there is a bug involving the return statement, I'll have to create a new variable to print it to the screen or inspect it with the debugger. Then I'm left with a decision to delete the variable before making a commit. IMO you might as well create that variable the first time around so when bugs come up you don't have to think about cleaning up the lines modified during debugging. Also now that PHP 8 is mainstream, optional chaining and null coalescing should be used as much as possible. It is faster and safer to always expect null values rather than occasionally checking.
I believe, there's ->json() method of the $response. Also, one could use Http:: get(...)->throw(). It wouldn't throw plain \Exception, however, but that could be even better (depends on the specification)
Also I have never seen anyone compare a variable to empty array. I'm not even sure if that reliably works! In some languages, the use of an empty array or empty object gives you back a newly allocated block of memory, and thus these checks actually fail. Just use empty() which also encapsulates the isset check
I like to put the api url in the env file as well. Then make a separate class to communicate with the api, and make giveConfig binding through the container
@@ercog7921 I think the API URL should be local to the code that uses it. If there are two APIs, then it is fine to use the environment name from .ENV to decide which one to use.
Theoretically, you could do an "early return" in the forach loop by reversing the if condition and using "continue" (instead of an actual return). If someone reading this thinks that would be considered a bad practice, please let me know!
I love using collections instead of an iteration to check values. Specially so I can perform a where () or even better, a first() to return the result.
Listening you was like: "i'm pretty sure, he is reading my mind..." haha Is great to know that i would refactor tha pretty same things of a experient programmer like you.
Nested IFs drive me crazy. It pretty much shows that the coder's approach to solving problem is to handle it sequentially instead of finding a pattern and grouping them together. If new criteria were to be introduced in the future, it will either add another level of IF or end up breaking the code, making the end result unpredictable. Also, in situations where database queries are involved, often the IF conditions can be coded directly into query builder instead of having to use an IF block to handle. e.g. $query->when($cond01, function ($q) { $q->where } . . . // or get / delete ->update([ . . . ]);
Very nice refactoring! Thanks for the hint with Laravel Pint and the article! The original code was frightening though.. and by no means this was caused by Laravel :-D This is purely inefficient coding (no SOLID principle, no DRY etc.)
I don't believe he's a senior developer except he believe he's senior because he's coding for 5 years or so. Also for Laravel yes sometimes I blame on Laravel because it's so easy that makes some devs may begin to use this tool before actually learning OOP and the language itself and they start working and destroy everything. But if we look into something like Symfony, it's so hard for any dev to start using it if he doesn't know OOP very well and the language itself, so we don't see this mess from a Symfony developer. That's my own opinion, the magic of laravel is a two edged sword.
Again, it's not framework's fault because developer is not experienced. Framework itself provides tools for developing and what you said only proves that Laravel is much better developed and easier to setup and use than Symphony. I don't want to setup framework for hours, I want to use it to create my business logic and Laravel allows me to do so much faster than Symphony. If I was a bad dev, I would create the code in exact same way using Symphony as well, however, I'd have to annoy myself for hour setting it up to do what I want. There are thousands of bad devs using Symphony or any other framework, the same way as there are using Laravel. Bad devs are everywhere
@@Novica89 But i think beginners should start there career on CORE Php or any other framework than Laravel for better growth and OOP skills, If a DEV start their career on Laravel it takes lot of time for them to learn good coding approach as laravel gives every thing without any effort. I totally agreed with @B&W on this
@@Novica89 Im not against Laravel, I love laravel, I also start my career on laravel, but i always thinks I am weak in OOP concepts, and i think it could be better If i started my career on CORE PHP
@@mubasharrashid1201 I'd say you're somewhat correct :) Having a firm(er) grasp on PHP OOP would be the best possible scenario before using any framework, but there is also a LOT that you can learn about OOP as you start using an advanced framework like Laravel :) There are so many advanced OOP concepts in the core of Laravel framework that, as you read it's whole documentation (which i would strongly advise) sticks into your brain and makes you rethink what you did and how you did it before you started using Laravel :) If you are somewhat intermediate in PHP and have an "ok" foundation in OOP, you could start with any framework from that point on and just learn more about OOP as you go with the frameworks help. There is so much to learn from the documentation itself, not to mention Laracasts and similar websites.
I think, since the if conditions only exists to throw exceptions, why not get rid of the if and use throw_if(), Code would be much shorter and meaningful Line 19-20 could be done like this, same with line 24-26 `throw_if(!$response->successful(), new \Exception('Error while getting data from google API'));` Great work though! I enjoy watching your video's, They always have something new that I learn from.
if we see this type of code we give the previous developer benefit of doubt by saying, 1. perhaps he was not in a sound mental state when he coded, 2. his environment was not favourable. 3. maybe he had to finish the whole module by the eod, and never got the opportunity to return to it, lol
I'm a junior dev with no prior commercial experience yet just finished school. But I learned the concept of early returns a few months ago from this channel and I would have never written such unreadable and ugly code.
And what in case if $json doesn't contain destination_address property somehow? That foreach loop is going to break. You either need additional check if it is set, or something like $destinationAddresses = $json->destiantion_addresses ?? []; foreach ($destinationAddresses as $destination) { ... }
Nice video and explanation @pavel 🙂 - one question I actually exactly follow your approach for setting API keys, in env and config file then reference the config value...but I have seen other devs directly referencing the env value in the code e.g. in a controller or method using env('API_KEY_NAME') -- I wonder is this a bad practice or what might be the pros/ cons of this ?
You're doing it right! The devs you've seen are doing it the wrong way. Generally if you use env variables they should also be declared in a config file. Povilas is right, by referencing env variables directly you won't be able to cache them and in some server/PHP configurations you won't even be able to retrieve them using the env() method.
@@free2idol1 I have VPS with almost default nGinx and php-fpm 8+ setup and I can confirm, that using env() in controller simply doesn't work. For example: dd( env('LOG_LEVEL') ) return null. I always use env() only in config files.
Рік тому
Second if can be replaced by empty($json->rows[0]->elements), also I'd import Exception. I'd also save destinations to a variable: $destinations = $json->destination_addresses ?? [];
well its pretty clear that the person who wrote the original code was a Junior dev... or maybe even a STUDENT... or something... definitely NOT a Mid/Senior dev... the 3 levels of IFs for doing OR/AND checks is a clear indication of that.
i consider myself as junior dev but i will never write that code in the way that senior write it (WTF is that tree of if else ) from the biginning (3 Years now ) i learn from your code styles and im a fun of it simple, clean and easy thanks i may call myself senior after that haha love you 😅😅😅😅😅😅😅😅😅😅😅😅
That comment is brainless. If that comment came from a senior dev, they too need to be questioned about their expertise cos it makes absolutely no sense.
The capturing of the exception, and then rethrowing it as a more generic exception is actually hurting the code even more. You lose the granularity that exceptions provide, and more specifically the ease of handling elsewhere. Personally I would have made separate exceptions for the google api one and the postal code not valid that way you are able to capture it higher up in the code if necessary, and not rely on a generic catch(Exception) block and reading the message because what happens if the text changes slightly? Then your condition is now dead. Personally I would have bumped the double or (three conditioned if statement) into a private helper function because otherwise it feels like a run on sentence when reading it out loud. Instead, I would just have it named "hasValidResults" and go from there. Also, this really doesn't belong as a trait, at that point it may as well be a service that you import with an interface.
I don't like "return" in loops. I would put above the loop $destination = null; variable. And if not found (null) throw exception. After the end of the cycle or break cycle. And the found, required value would be left as the last return. p.s. url to env or at leas class constant.
Do you mean the way of the "happy path", where your desired return value is always on the end of the method, so everything above is for operating on the data (like validating or transforming it)? For me this method is the best, coz it saves some energy for the future self: Imagine that now you need to do some more transforming within the loop - or even better: you want to move the loop insides into other method or even class - like a filter class, that it's main goal would be to get an object depending on the destination iso code. Something, that will allow you to get an object or in a case, there no condition will be met - exception or null value will be provided. Not only it will shorten the main code, the responsibility of the code will be spread more even, but also by using some KISS implementation you won't need to know exactly what happens in this new method, coz the name will cover it.
why do you have conditions in the block || instead of && ? for example, if !isset($json->rows) returns false, what's the point of checking the same property further to see if it's an array?
Code has to be at least readable, not that clean as many people say. Sometimes clean code is confusing, we have to find some reasonable balance between clean code and readable code. I know sometimes we code so fast and under pressure so it apparently work's, but we have to refactor the code, because probably later we have to change something and it becomes a nightmare.
Clean code and readable code are pretty much the same thing. The code at the start was neither, the code at the end was both. So I don't really get your point.
IMO... 1.API URL should be in a constant string in the class. 2. Function return should be a data object. 3. Function return should be strongly typed. 4. It should use custom exceptions so that the caller can distinguish between different exceptions. P.S. I would not hire this coder. The quality of this code is not what i would expect from a junior developer much less someone claiming to be a senior developer.
Great video, but do You really need multiple isset validation ? I mean `if (!isset($json->rows[0]->elements))` should be enough or am I missing something ? also whole json validation is strange in this code (for example there is no checking of destination_addresses existence but I suppose that You wanted to show how to refactor code not how to bug hunt
why iterating over $json->destination_addresses if you can just use one line of php internal function: _if ( _*_in_array_*_ ('UK', $json->destination_addresses) === true ) { ..._ 😉
Senior labels are overrated, just because you've been doing this for years doesn't mean you've improved any lol really depends on the company you work for, does it challenge you, are there people more knowledgeable there telling you how to do things right? stuff like that. If there aren't any, or their seniors don't care about helping the younger devs then you're not going to improve no matter how long you've worked there so you can technically be a senior dev of 10+ years and still be making rookie mistakes. This isn't even that bad to be honest, i've seen considerably worse, the nested if's are nasty but i've seen this kind of stuff in the middle of the HTML in the days before Laravel so yeah, that was worse lol
Why people blame framework for bad code? I have 2 takeways 1. Why enviroment values are received via config helper not env helper? 2. When can I make joint condition statements in early return approach?
I do not know how it call it senior i said alot says where are elite developer but just do not knowing any thing i was worked with company last year on laravel backend and it has like alot of problem in performance and in security and not just that but even on database level no relations at all
@@underflowexception Yeah exactly, it also works on true/false and 0/1, so if the concern is about making code shorter and readable, better to use single condition. I just mentioned as Sir @povilasKorop asked to share more suggestions..
Seeing this code coming from a senior dev makes me feel really good about myself
sometimes they just simply won't adapt to a new concept
The person that wrote that code is not a senior dev. They might call themselves senior, but they're not.
Perhaps it was a purist who thinks functions are only allowed to have one return statement.
Senior dev as in 90yo PHP dev who's been coding for 2 weeks
😂same here
I have 20+ years of experience. I met every type of developer you could ever meet.
Once I worked with a senior developer who didn't believe in indentation. He wrote code on few lines and even mocked me when I indented his code every time I had to correct one of his bug.
On another occasion, another colleague, I had to search for a bug in one of his controllers, that was more than 1000 lines long.
In the past I made mistakes, of course, learning is a "long" process, but I always tried to do my best to write clean code.
Sometimes other people just don't seem to care.
Tell me about it, rigth now Im working on a proyect with clases with +3000 lines each, is a mess!!!
@@GerardoBelot uwoww, is that a legacy project ?
Lol, I can feel it... In my freelance journey, I had worked and still working with a team of devs, where most guys really don't care about the pain of others. It mostly happens when the most senior one ignores it.
@@GerardoBelot i feel you bruh, im in the middle of refactor it also, and its a niggghhtttmaarreee
Feel you bro, some people are just fine as long as the functionality works...
json_decode($response->body()) can be refactored into $response->json()
Nice demonstration of how nested if-elses can be improved drastically! I hope the author would see this video and learn from it.
I new versions of laravel? I think that Guzzle remove the json() method because it was not in the specs of PSR, but i am not sure if the laravel facade Http also remove the method
@@JoseDlucca I confirm $response->json() works just fine in Laravel version 10.12
You can even take some nested object in the JSON like this:
$json = $response->json('rows.0.elements.0') and it will return null if that object doesn't exist
@@spicynoodle7419thanks for sharing 🙏
Works with Laravel 8 too@@celucasa6810
People that blame a framework for that code simply lack in judgement and honesty. Every framework AND language allows bad coding.
Blaming the framework for bad code is like blaming a hammer not working as a saw.
Seniors sometimes write code which smells. But before making any conclusions I think there should be some clarification questions regarding this code, for example:
- Does the code work?
- How well does it work?
- Maybe there was some deadlines to write this code?
- Is release was more important than quality?
You have to consider this and probably much more questions together. So "Keep Calm and continue refactoring your codebase to better code and design"
That is true, but if You are 5 levels deep in nested if/else then You should consider that something is wrong :) but as You mentioned there are multiple reasons for this code to exist
tbf, if your task wasn't to write shit, this code is not acceptable, because it is much easier to write better code than this.
Nah, I don't see any of these excuses valid for this case. This code IS garbage.
One of the point of "seniority" is to know how to write clean code "already".
I can't imagine that monster CEO who was standing behind, yelling "FASTER YOU BASTARD!!!!" and that "senior" suddenly forgot early returns concept.
More logical for me is sabotage before quitting 😂
Man, I just love these types of questions that need refactor. I would refactor code all day.
That means you are not stuck 😅
I believe the only aspect I would approach differently is implementing a service class specifically for the Google Service. However, I must acknowledge that your version of the code is more streamlined and organized.
It should definitely reside within a service class, and extracting the code into the service is not in collision with having the streamlined and organized code.
He probably didnt do it to keep the video short.
it is so satisfying how nested if-else code collapses with early returns...
I think the foreach could be changed with an in_array, that also can be early returned. Then at the end only return what you need so the logic is fully complete.
i don't think it would work as in_array will search for the whole destination value..but in this case you just want a part of the destination so strpos/str_contains is just fine
You could even simplify the joined if statement to just one check with null-safe operator:
if ( empty($json?->rows[0]?->elements) ) { }
I would have done the same, except extracted only the elements since other parts of the response aren't used:
$json = $response->json('rows.0.elements.0');
if (empty($json)) {
throw...
}
It only works on PHP 8 😢
I would add one more change: use a different message in the last exception. The second exception (after the refactor) is when you don't get well-formed data in the response. That's different from not getting any UK destinations (the third exception). If all you have is an error in a log, it's nicer if you get a message like "no UK destination received", then you know where it went wrong.
You could have moved the foreach into the IF block above with the 3 conditions and made them positive checks instead. So if all the checks passed, the loop would run and possibly return. Else, the Exception would be thrown at the very end.
Just a minor style change and removes the duplicated Exception
such a nice way to explain each and every aspects of line.
I am not a senior developer, but I have had the chance of working on projects developed by seniors, which gave me such a great opportunity of refactoring, especially making controllers
issets can also be combined like this:
! isset($json->rows, $json->rows[0]->elements)
Thanks! Never found (or looked up) this in the docs, but it is! BTW you can only check the 2nd variable, as isset returns false, if the rows property or an index on it is not set, it does not throws exception. (It is also new for me, that isset is like nullsafe at any part, but tested in PHP sandbox now back to 5.2.0, it worked - never tried this before (~13 years in PHP development...))
@@EndreVarga_rnd Haha there's always a room for an improvement.
This is not from a senior developer, this is from an impostor
Senior is probably referring to the amount of years he has been programming instead his level of expertise.
@@He4vyD Yes, definitely
I googled the word "impostor", means liar。You think Laravel Daily is a liar? Why?
@bubblelong4759 the commenter is talking about the person who wrote the code being an imposter(fake) as a senior, accusing them of not being as competent as a senior developer would be.
the commenter has not made a comment on laravel daily
😂😅
My instinct was saying after seeing your retweet that you’re definitely gonna make a video on that 😂
Now here we are!
Couldn't miss that one, huh! :)
@@LaravelDaily enjoyed every second of this video 💫
You can search for the "UK" value in the destinations array by using the in_array function :), in stead of using the foreach loop :)
Beautifully done by you!
Great learning be continue with these types of code refracting videos.
I would use Laravel throw_if() function instead of the IF statements to make the code even more elegant, readable and somewhat with less lines letting it all in Laravel house of tools. Thank you for another Great Video!
Excellent video. I always tell my web team "write your code under the assumption that the dev who inherits it is a homicidal maniac who knows where you live". 👏
Thank God. I was scared of my coding style until I watched this video.
Very good, I'm a php programer and I use laravel too, learning so much with your channel, my congrats. Sorry my english.
This kind of refactoring videos helps a lot thank you ;)
Nice, it removes a lot of mental burden. One more thing I would is the foreach section I would extract it in a separate method let's call it UKDestination() and the I would move the last exception throw above,
$UKDestination = $this->UKDestination($parameters);
if ( empty($UKDestination)) {
throw \Exception(''postal code not valid')
}
return $UKDestination
Maybe it's just me but I prefer the happy path to be always at the bottom otherwise it messes my brain up . Thanks!
Yeah, good suggestion! I was actually thinking to use some collection operation instead of a longer foreach loop. But separate method also makes sense!
@@LaravelDaily Thanks!
Wow, I'm amazed by the level of the comments here. Makes me feel good that I worked in a few places where developers were kind and cared a lot for helping and improving and trying to understand the situations we're at sometimes.
The post itself mentions they received an application. That is kinda open, isn't that? If that's an application for a dev role there can be a looot of stuff underneath it. What was the deadline? Was it live coding? There are a lot of developers who crumble under the pressure of having someone on their shoulders, which is the same as live coding. Same goes for a very short deadline. How much code was expected in the given timeframe? There are a lot of job applications that give you a deadline that you are not expected to meet without leaving stuff behind, I've had a few in the past.
Also the person exposing it was posting it to feel superior and get engagement from people that would also want that - which is pretty obvious given, you know, it's from twitter.
Judging other people's skill for some random code you see without context isn't useful for anyone is not healthy at all.
this video should be wateched by everybody join a company, on each level of seniority!
Only thing that I'd change here is moving common part of that URL to .env file.
By "common part" I mean everything from start to "api".
Yes! Then if there's a sandbox version of their api, you can point to that version easily
one of my pet peeves is initializing values in the return statement. every time there is a bug involving the return statement, I'll have to create a new variable to print it to the screen or inspect it with the debugger. Then I'm left with a decision to delete the variable before making a commit.
IMO you might as well create that variable the first time around so when bugs come up you don't have to think about cleaning up the lines modified during debugging.
Also now that PHP 8 is mainstream, optional chaining and null coalescing should be used as much as possible. It is faster and safer to always expect null values rather than occasionally checking.
I believe, there's ->json() method of the $response. Also, one could use Http:: get(...)->throw(). It wouldn't throw plain \Exception, however, but that could be even better (depends on the specification)
Also I have never seen anyone compare a variable to empty array. I'm not even sure if that reliably works! In some languages, the use of an empty array or empty object gives you back a newly allocated block of memory, and thus these checks actually fail.
Just use empty() which also encapsulates the isset check
He is most likely a senior javascript developer.... seen those empty arrays all over js code....
I like to put the api url in the env file as well. Then make a separate class to communicate with the api, and make giveConfig binding through the container
IMO API URL should NOT be in .ENV but it should be in a constant string in the class..
@@SodalisUK api url usually has live and sandbox. Therefore, in order not to make 2 constants and write if logic, it is easier to use env
@@ercog7921 I think the API URL should be local to the code that uses it. If there are two APIs, then it is fine to use the environment name from .ENV to decide which one to use.
Theoretically, you could do an "early return" in the forach loop by reversing the if condition and using "continue" (instead of an actual return). If someone reading this thinks that would be considered a bad practice, please let me know!
I love using collections instead of an iteration to check values. Specially so I can perform a where () or even better, a first() to return the result.
Listening you was like: "i'm pretty sure, he is reading my mind..." haha
Is great to know that i would refactor tha pretty same things of a experient programmer like you.
We can use also throw_if for one liner condition.
वो क्या है ना, जो डिमांड होता है हैं, चर्चा भी उसी का होता है.. This is the demand and users of laravel framework.. 💙
Nested IFs drive me crazy.
It pretty much shows that the coder's approach to solving problem is to handle it sequentially instead of finding a pattern and grouping them together.
If new criteria were to be introduced in the future, it will either add another level of IF or end up breaking the code, making the end result unpredictable.
Also, in situations where database queries are involved, often the IF conditions can be coded directly into query builder instead of having to use an IF block to handle.
e.g.
$query->when($cond01, function ($q) {
$q->where
}
.
.
.
// or get / delete
->update([
.
.
.
]);
Very nice refactoring! Thanks for the hint with Laravel Pint and the article!
The original code was frightening though.. and by no means this was caused by Laravel :-D This is purely inefficient coding (no SOLID principle, no DRY etc.)
I don't believe he's a senior developer except he believe he's senior because he's coding for 5 years or so.
Also for Laravel yes sometimes I blame on Laravel because it's so easy that makes some devs may begin to use this tool before actually learning OOP and the language itself and they start working and destroy everything.
But if we look into something like Symfony, it's so hard for any dev to start using it if he doesn't know OOP very well and the language itself, so we don't see this mess from a Symfony developer.
That's my own opinion, the magic of laravel is a two edged sword.
I strongly agree with you mate!
Again, it's not framework's fault because developer is not experienced. Framework itself provides tools for developing and what you said only proves that Laravel is much better developed and easier to setup and use than Symphony. I don't want to setup framework for hours, I want to use it to create my business logic and Laravel allows me to do so much faster than Symphony. If I was a bad dev, I would create the code in exact same way using Symphony as well, however, I'd have to annoy myself for hour setting it up to do what I want. There are thousands of bad devs using Symphony or any other framework, the same way as there are using Laravel. Bad devs are everywhere
@@Novica89 But i think beginners should start there career on CORE Php or any other framework than Laravel for better growth and OOP skills,
If a DEV start their career on Laravel it takes lot of time for them to learn good coding approach as laravel gives every thing without any effort.
I totally agreed with @B&W on this
@@Novica89 Im not against Laravel, I love laravel, I also start my career on laravel, but i always thinks I am weak in OOP concepts, and i think it could be better If i started my career on CORE PHP
@@mubasharrashid1201 I'd say you're somewhat correct :) Having a firm(er) grasp on PHP OOP would be the best possible scenario before using any framework, but there is also a LOT that you can learn about OOP as you start using an advanced framework like Laravel :) There are so many advanced OOP concepts in the core of Laravel framework that, as you read it's whole documentation (which i would strongly advise) sticks into your brain and makes you rethink what you did and how you did it before you started using Laravel :) If you are somewhat intermediate in PHP and have an "ok" foundation in OOP, you could start with any framework from that point on and just learn more about OOP as you go with the frameworks help. There is so much to learn from the documentation itself, not to mention Laracasts and similar websites.
I think, since the if conditions only exists to throw exceptions, why not get rid of the if and use throw_if(), Code would be much shorter and meaningful
Line 19-20 could be done like this, same with line 24-26
`throw_if(!$response->successful(), new \Exception('Error while getting data from google API'));`
Great work though! I enjoy watching your video's, They always have something new that I learn from.
I have experienced this too. The senior who handled the app made the app like and feel like a wordpress :)
amazing, thank you for sharing
I might be wrong, but !isset($json->rows) || json->rows == [] might be replaced by empty($json->rows).
if you use larastan then it will shout at you if you use empty() these days; so you should probably avoid it
No matter what framework or language. Anyone who wrote these if else hell, should not be a senior developer. So, don't blame on code or framework.
if we see this type of code we give the previous developer benefit of doubt by saying,
1. perhaps he was not in a sound mental state when he coded,
2. his environment was not favourable.
3. maybe he had to finish the whole module by the eod,
and never got the opportunity to return to it, lol
I'm a junior dev with no prior commercial experience yet just finished school. But I learned the concept of early returns a few months ago from this channel and I would have never written such unreadable and ugly code.
This channel has been recommended by 2 jobs I've worked at already. You're on the right path as a junior.
@@lynic-0091 ❤️
Don't judge like this. Today's young generation has way more available resources than it was 20 or even 10 years ago.
@@DimaSimonishvili As a programmer you're supposed to stay up to date. You can learn and master code style like this easily within a year or two.
@@lynic-0091 I fully agree but not everyone has the same capabilities.
Add validation function for response))
You fixed code like an artist ☺️
Awww thanks :)
@@LaravelDaily Hi, I know Laravel little bit, can you please suggest me some roadmap or course for a beginner to develop projects and APIs in Laravel?
Here's my public Laravel roadmap: laraveldaily.com/roadmap-learning-path
@@LaravelDaily Thank you so much, I will check that out 👍
And what in case if $json doesn't contain destination_address property somehow?
That foreach loop is going to break.
You either need additional check if it is set, or something like
$destinationAddresses = $json->destiantion_addresses ?? [];
foreach ($destinationAddresses as $destination) {
...
}
catching it and throwing it again! 🤣
Nice video and explanation @pavel 🙂 - one question I actually exactly follow your approach for setting API keys, in env and config file then reference the config value...but I have seen other devs directly referencing the env value in the code e.g. in a controller or method using env('API_KEY_NAME') -- I wonder is this a bad practice or what might be the pros/ cons of this ?
I think it's related to cache, with this approach you can't cache config.
You're doing it right! The devs you've seen are doing it the wrong way. Generally if you use env variables they should also be declared in a config file. Povilas is right, by referencing env variables directly you won't be able to cache them and in some server/PHP configurations you won't even be able to retrieve them using the env() method.
@@vanosgarage thanks... May I know which server/PHP configuration where env() cannot be used?
@@free2idol1 I have VPS with almost default nGinx and php-fpm 8+ setup and I can confirm, that using env() in controller simply doesn't work. For example: dd( env('LOG_LEVEL') ) return null. I always use env() only in config files.
Second if can be replaced by empty($json->rows[0]->elements), also I'd import Exception. I'd also save destinations to a variable: $destinations = $json->destination_addresses ?? [];
I would do the tow early if statements in tow lines without the brackets and save more 4 lines
well its pretty clear that the person who wrote the original code was a Junior dev... or maybe even a STUDENT... or something... definitely NOT a Mid/Senior dev... the 3 levels of IFs for doing OR/AND checks is a clear indication of that.
You nailed it.
i consider myself as junior dev but i will never write that code in the way that senior write it (WTF is that tree of if else ) from the biginning (3 Years now ) i learn from your code styles and im a fun of it simple, clean and easy thanks i may call myself senior after that haha love you 😅😅😅😅😅😅😅😅😅😅😅😅
That comment is brainless. If that comment came from a senior dev, they too need to be questioned about their expertise cos it makes absolutely no sense.
If that guy is a senior then I'm the ruler of the galaxy. 🤣🤣
I'm afraid the client hired a junior who's pretending he's a senior.
The capturing of the exception, and then rethrowing it as a more generic exception is actually hurting the code even more. You lose the granularity that exceptions provide, and more specifically the ease of handling elsewhere. Personally I would have made separate exceptions for the google api one and the postal code not valid that way you are able to capture it higher up in the code if necessary, and not rely on a generic catch(Exception) block and reading the message because what happens if the text changes slightly? Then your condition is now dead.
Personally I would have bumped the double or (three conditioned if statement) into a private helper function because otherwise it feels like a run on sentence when reading it out loud. Instead, I would just have it named "hasValidResults" and go from there.
Also, this really doesn't belong as a trait, at that point it may as well be a service that you import with an interface.
I would also remove the curly braces, since they're not needed.
I don't like "return" in loops. I would put above the loop $destination = null; variable. And if not found (null) throw exception. After the end of the cycle or break cycle.
And the found, required value would be left as the last return.
p.s. url to env or at leas class constant.
Do you mean the way of the "happy path", where your desired return value is always on the end of the method, so everything above is for operating on the data (like validating or transforming it)? For me this method is the best, coz it saves some energy for the future self: Imagine that now you need to do some more transforming within the loop - or even better: you want to move the loop insides into other method or even class - like a filter class, that it's main goal would be to get an object depending on the destination iso code. Something, that will allow you to get an object or in a case, there no condition will be met - exception or null value will be provided. Not only it will shorten the main code, the responsibility of the code will be spread more even, but also by using some KISS implementation you won't need to know exactly what happens in this new method, coz the name will cover it.
the try catch and throw inside the catch killed me, i guess you could then try and catch that method and throw and continue until the end of times...
I can't believe that this is a Senior code.
Thanks a lot.
why do you have conditions in the block || instead of && ?
for example, if !isset($json->rows) returns false, what's the point of checking the same property further to see if it's an array?
Code has to be at least readable, not that clean as many people say. Sometimes clean code is confusing, we have to find some reasonable balance between clean code and readable code. I know sometimes we code so fast and under pressure so it apparently work's, but we have to refactor the code, because probably later we have to change something and it becomes a nightmare.
Clean code and readable code are pretty much the same thing. The code at the start was neither, the code at the end was both. So I don't really get your point.
I would not remove the try/catch block. The rest is fine by me :)
IMO...
1.API URL should be in a constant string in the class.
2. Function return should be a data object.
3. Function return should be strongly typed.
4. It should use custom exceptions so that the caller can distinguish between different exceptions.
P.S. I would not hire this coder. The quality of this code is not what i would expect from a junior developer much less someone claiming to be a senior developer.
Great video, but do You really need multiple isset validation ? I mean `if (!isset($json->rows[0]->elements))` should be enough or am I missing something ? also whole json validation is strange in this code (for example there is no checking of destination_addresses existence but I suppose that You wanted to show how to refactor code not how to bug hunt
Nice one!
Thanks for all you do povilas...From the timestamp 2:41 can't I just use the global env function to reference my api key?
Because config caching wouldn't work this way if you need it
I refuse to believe that this code was written by senior dev, no way
why iterating over $json->destination_addresses if you can just use one line of php internal function: _if ( _*_in_array_*_ ('UK', $json->destination_addresses) === true ) { ..._ 😉
🤔 does being a senior synonym of perfect?
That's obviously a junior/trainee's code. No freaking way a senior would do such insane mistakes.
Senior labels are overrated, just because you've been doing this for years doesn't mean you've improved any lol really depends on the company you work for, does it challenge you, are there people more knowledgeable there telling you how to do things right? stuff like that.
If there aren't any, or their seniors don't care about helping the younger devs then you're not going to improve no matter how long you've worked there so you can technically be a senior dev of 10+ years and still be making rookie mistakes.
This isn't even that bad to be honest, i've seen considerably worse, the nested if's are nasty but i've seen this kind of stuff in the middle of the HTML in the days before Laravel so yeah, that was worse lol
@@liquidsnake6879 you've seen worse? dayum
I would ask how did this code get through any code review in the first place.
need a sequence of senior code reviews
Code that works with API should not be in trait, maybe encapsulated in driver class
Very Good Thanks.
Why people blame framework for bad code?
I have 2 takeways
1. Why enviroment values are received via config helper not env helper?
2. When can I make joint condition statements in early return approach?
1. To cache the config
2. It depends on the scenario, there's no single answer
@@LaravelDaily To avoid nulls in env values when is cached.
I honestly feel bad for whoever wrote this code, it's all over the internet 😅
When we return in looping, it will automatically stop the whole the loop right?. not making the looping pending?
Yes probably. But it depends on the situation.
great refactoring! i have a question: is it really safe to save keys/passwords in a config file? isn't it better to have them in the db?
That's your personal preference.
I do not know how it call it senior i said alot says where are elite developer but just do not knowing any thing i was worked with company last year on laravel backend and it has like alot of problem in performance and in security and not just that but even on database level no relations at all
This mistake can be very costly.
I hope the original author of that code watched this video and took some notes.
oh yes the old hadouken code
why is the array being cast to object ?
Probably because he use the data in php, not outside, and he preffer calling it like $data->destination instead of $data['destination']
in your opinion what is best to save address field in address column or separate them for country state street complex /house number etc....
It depends on what operations you're planning to do with those address fields in the future. I usually keep them separately.
very helpful.
Very nice ❤
The funny part is that Google Maps API resolves 99% of these issues in even fewer lines of codes.
How do you format blocks of code in PhpStorm?
It formats it automatically
@@LaravelDaily Thanks
Wow this is my code... Joke of course 🤣
this is how students code at school
There is still a duplicated exception message
Good point, another potential improvement.
instead of:
if ( ! isset( $json->rows ) || $json->rows == [] )
can't we use?
if ( empty( $json->rows) )
empty checks if the value is null AND the if the array has elements, isset just checks if the value is set regardless of the value
It should work fine with empty()
@@underflowexception Yeah exactly, it also works on true/false and 0/1, so if the concern is about making code shorter and readable, better to use single condition. I just mentioned as Sir @povilasKorop asked to share more suggestions..
The famous “Hadouken” of ifs… and the dev who wrote it is a senior???