Refactor "Senior" PHP Code with Early Returns 

Seeing this code coming from a senior dev makes me feel really good about myself

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.

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.

People that blame a framework for that code simply lack in judgement and honesty. Every framework AND language allows bad coding.

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"

Man, I just love these types of questions that need refactor. I would refactor code all day.

it is so satisfying how nested if-else code collapses with early returns...

This is not from a senior developer, this is from an impostor

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.

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". 👏

such a nice way to explain each and every aspects of line.

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.

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.

Great learning be continue with these types of code refracting videos.

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 ;)

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

You could even simplify the joined if statement to just one check with null-safe operator: if ( empty($json?->rows[0]?->elements) ) { }

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 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.

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!

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.

amazing, thank you for sharing

this video should be wateched by everybody join a company, on each level of seniority!

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.

You can search for the "UK" value in the destinations array by using the in_array function :), in stead of using the foreach loop :)

My instinct was saying after seeing your retweet that you’re definitely gonna make a video on that 😂 Now here we are!

issets can also be combined like this: ! isset($json->rows, $json->rows[0]->elements)

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)

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.)

You nailed it.

Very Good Thanks.

Thanks a lot.

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!

I have experienced this too. The senior who handled the app made the app like and feel like a wordpress :)

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.

Nice one!

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 ❤

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".

very helpful.

वो क्या है ना, जो डिमांड होता है हैं, चर्चा भी उसी का होता है.. This is the demand and users of laravel framework.. 💙

We can use also throw_if for one liner condition.

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.

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

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'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.

You fixed code like an artist ☺️

catching it and throwing it again! 🤣

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

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

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?

in your opinion what is best to save address field in address column or separate them for country state street complex /house number etc....

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!

Add validation function for response))

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?

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) { ... }

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 might be wrong, but !isset($json->rows) || json->rows == [] might be replaced by empty($json->rows).

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.

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?

I would ask how did this code get through any code review in the first place.

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 😅😅😅😅😅😅😅😅😅😅😅😅

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

When we return in looping, it will automatically stop the whole the loop right?. not making the looping pending?

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.

need a sequence of senior code reviews

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 not remove the try/catch block. The rest is fine by me :)

I can't believe that this is a Senior code.

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.

How do you format blocks of code in PhpStorm?

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.

I would also remove the curly braces, since they're not needed.

The funny part is that Google Maps API resolves 99% of these issues in even fewer lines of codes.

oh yes the old hadouken code

I honestly feel bad for whoever wrote this code, it's all over the internet 😅

instead of: if ( ! isset( $json->rows ) || $json->rows == [] ) can't we use? if ( empty( $json->rows) )

I hope the original author of that code watched this video and took some notes.

That's obviously a junior/trainee's code. No freaking way a senior would do such insane mistakes.

🤔 does being a senior synonym of perfect?

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

why is the array being cast to object ?

Is it not better to call that api on client side?

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 developer with junior developer's flavour 🤪🤪🤪🤪

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 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.

The famous “Hadouken” of ifs… and the dev who wrote it is a senior???

This mistake can be very costly.

Wow this is my code... Joke of course 🤣

this is how students code at school

There is still a duplicated exception message

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.

Code that works with API should not be in trait, maybe encapsulated in driver class

I refuse to believe that this code was written by senior dev, no way

I know PHP in general has this reputation of bad code because of the lower barrier of entry and the fact that it has massive popularity. If you work in Java or C# or other languages normally used in corporate environments, you only end up writing code after you've been vetted, hired, trained, and then double checked by seniors so of course it's going to end up with better code. But in the last years with other languages getting more popular, I've seen code that was just as bad or worse than what you'd normally find in PHP.

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?