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 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
@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 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.
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 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 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
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.
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.
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.
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
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.
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.
The early return / fail fast method resembles unit tests - getting your ducks in a row! I understand where these nested statements come from - it's "stream of consciousness" programming, but each "test" leaves a micro tech-debt to be resolved. Full marks to the original developer in this example, to test for each point of failure along the way. In writing either code blocks or unit tests, I like to begin with pseudocode - a comment on each line, reading like a checklist of things to verify, then coding beneath each one.
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
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...))
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.
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!
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
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([ . . . ]);
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.
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!
Год назад
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 ?? [];
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 ) { ..._ 😉
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
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". 👏
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?
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.
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
If a guy from twitter consider this code as from senior, this is his own problem, this is clearly very junior code from someone who makes his first steps, nothing to do with laravel.
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)
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.
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.
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!
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
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.
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.
This sort of "problems" are there just because, maybe, this developers are used to work with other environments or frameworks or whatever language is, maybe is just that they know basical laravel or even php, and since they don't fully understand all the things we know from profesor povilas, maybe they are lacking of options to code like this example. Maybe is just a matter of experience.
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 😅😅😅😅😅😅😅😅😅😅😅😅
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.
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?
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.)
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
You can search for the "UK" value in the destinations array by using the in_array function :), in stead of using the foreach loop :)
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.
Very Good Thanks.
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.
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
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
😂😅
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.
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.
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 😂
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.
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 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
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.
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.
Man, I just love these types of questions that need refactor. I would refactor code all day.
That means you are not stuck 😅
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.
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
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.
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.
The early return / fail fast method resembles unit tests - getting your ducks in a row! I understand where these nested statements come from - it's "stream of consciousness" programming, but each "test" leaves a micro tech-debt to be resolved. Full marks to the original developer in this example, to test for each point of failure along the way.
In writing either code blocks or unit tests, I like to begin with pseudocode - a comment on each line, reading like a checklist of things to verify, then coding beneath each one.
I refuse to believe that this code was written by senior dev, no way
Add validation function for response))
it is so satisfying how nested if-else code collapses with early returns...
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
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.
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! 🤣
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
why not use \Throwable instead of \Exception ?
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.
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
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!
Whats more funny is that someone calls himself a senior and cry about his ugly code..
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....
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([
.
.
.
]);
I don't get a job because "lack of experience". But if this code is written by Senior developer, I'm over qualified.....
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.
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!
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 ?? [];
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 ) { ..._ 😉
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
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". 👏
You can kill somebody using hammer. Then the hammer is bad or person witch use this hammer? Hammer is tool === Laravel is tool.
Code that works with API should not be in trait, maybe encapsulated in driver class
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?
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.
such a nice way to explain each and every aspects of line.
Seriously ??
Is that a senior version code ???
Seem like a junior version to me
I would not remove the try/catch block. The rest is fine by me :)
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.
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
If a guy from twitter consider this code as from senior, this is his own problem, this is clearly very junior code from someone who makes his first steps, nothing to do with laravel.
Seems like a "senior" developer that used to work on position I am right now. Really - the code style is absolutely the same :/
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)
I would do the tow early if statements in tow lines without the brackets and save more 4 lines
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.
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 would also remove the curly braces, since they're not needed.
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 💫
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.
Thank God. I was scared of my coding style until I watched this video.
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!
The famous “Hadouken” of ifs… and the dev who wrote it is a senior???
this video should be wateched by everybody join a company, on each level of seniority!
We can use also throw_if for one liner condition.
This mistake can be very costly.
amazing, thank you for sharing
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
Great learning be continue with these types of code refracting videos.
This kind of refactoring videos helps a lot thank you ;)
Is this code really wrote by senior? 😂
🤔 does being a senior synonym of perfect?
Wow this is my code... Joke of course 🤣
I can't believe that this is a Senior code.
Beautifully done by you!
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.
$response->failed()
this is how students code at school
oh yes the old hadouken code
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 honestly feel bad for whoever wrote this code, it's all over the internet 😅
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.
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.
With 3 yrs of experience and been able to detect those bad practices that you refactor by myself could I say that I'm actually semisenior?
Semi-yes :)
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.
You nailed it.
Thanks a lot.
This sort of "problems" are there just because, maybe, this developers are used to work with other environments or frameworks or whatever language is, maybe is just that they know basical laravel or even php, and since they don't fully understand all the things we know from profesor povilas, maybe they are lacking of options to code like this example. Maybe is just a matter of experience.
Just like that
if (
empty($json->rows)
|| !isset($json->rows[0]->elements)
|| collect($json->destination_addresses)->doesntContain(fn ($destination) => str_contains($destination, 'UK'))
) {
throw ...
}
return ...
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 😅😅😅😅😅😅😅😅😅😅😅😅
The funny part is that Google Maps API resolves 99% of these issues in even fewer lines of codes.
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.
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.
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']
Senior developer with junior developer's flavour 🤪🤪🤪🤪
Xool
Nice one!
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.)