Refactor "Senior" PHP Code with Early Returns

[@hansherrera5321]

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

[@andibachtiar8788]

sometimes they just simply won't adapt to a new concept

[@ivanjelenic5627]

The person that wrote that code is not a senior dev. They might call themselves senior, but they're not.

[@PaulaBean]

Perhaps it was a purist who thinks functions are only allowed to have one return statement.

[@spicynoodle7419]

Senior dev as in 90yo PHP dev who's been coding for 2 weeks

[@lassestube]

😂same here

[@AranyaSen]

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.

[@JoseDlucca]

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

[@celucasa6810]

@@JoseDlucca I confirm $response->json() works just fine in Laravel version 10.12

[@spicynoodle7419]

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

[@1337kaas]

​@@spicynoodle7419thanks for sharing 🙏

[@tomsriver2838]

Works with Laravel 8 too​@@celucasa6810

[@f13775]

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"

[@cichy86]

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

[@Asiro-S]

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.

[@anti5895]

Nah, I don't see any of these excuses valid for this case. This code IS garbage.

[@JackSoutherner]

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 😂

[@ionelCristianLupu_]

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

[@evol9013]

That means you are not stuck 😅

[@IlPandax]

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.

[@GerardoBelot]

Tell me about it, rigth now Im working on a proyect with clases with +3000 lines each, is a mess!!!

[@ariumboroseno]

@@GerardoBelot uwoww, is that a legacy project ?

[@souravdutt7]

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.

[@rikiamaru]

@@GerardoBelot i feel you bruh, im in the middle of refactor it also, and its a niggghhtttmaarreee

[@javieru5871]

Feel you bro, some people are just fine as long as the functionality works...

[@matthiashoffmann6555]

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

[@wgblondel]

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

[@He4vyD]

Senior is probably referring to the amount of years he has been programming instead his level of expertise.

[@wgblondel]

@@He4vyD Yes, definitely

[@bubblelong4759]

I googled the word "impostor", means liar。You think Laravel Daily is a liar? Why?

[@phoneywheeze]

​ ​@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

[@ste8701]

😂😅

[@jessedeboer2301]

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.

[@ste8701]

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

[@laubannenberg5446]

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.

[@crispy-channel]

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

[@devKazuto]

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

[@spicynoodle7419]

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

[@evol9013]

It only works on PHP 8 😢

[@matrixlander85]

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

[@shahsawoodshinwari]

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

[@alnahian2003]

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

[@LaravelDaily]

Couldn't miss that one, huh! :)

[@alnahian2003]

@@LaravelDaily enjoyed every second of this video 💫

[@omkarchoudhary1899]

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

[@adrianoalves-qripto]

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!

[@GutoCmtt]

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.

[@asfiaaiman3699]

Beautifully done by you!

[@joeythai1000]

Very good, I'm a php programer and I use laravel too, learning so much with your channel, my congrats. Sorry my english.

[@al-assili-sounds]

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

[@wadecodez]

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.

[@enriquerobledo845]

This kind of refactoring videos helps a lot thank you ;)

[@LifeAquaticSteveZissou]

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

[@mityukov]

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)

[@JouvaMoufette]

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

[@Daaboo]

He is most likely a senior javascript developer.... seen those empty arrays all over js code....

[@dawoodfaiz2338]

Thank God. I was scared of my coding style until I watched this video.

[@mohamadcheaib]

We can use also throw_if for one liner condition.

[@giacomogaravaglia6742]

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

[@7error8lade]

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([ . . . ]);

[@DougLopes]

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.

[@ScottHorsley]

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.

[@coding_david]

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

[@Teddykiss1996]

Add validation function for response))

[@RhythmnOfThought]

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!

[@shubhamsahuSD]

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

[@_whatistruth]

catching it and throwing it again! 🤣

[@NemanjaSokolovic-k6b]

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

[@rage13]

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.

[@Rodrigoalvesdegois]

I might be wrong, but !isset($json->rows) || json->rows == [] might be replaced by empty($json->rows).

[@thirdvect0r]

if you use larastan then it will shout at you if you use empty() these days; so you should probably avoid it

[@birsingh5388]

You fixed code like an artist ☺️

[@LaravelDaily]

Awww thanks :)

[@birsingh5388]

@@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?

[@LaravelDaily]

Here's my public Laravel roadmap: laraveldaily.com/roadmap-learning-path

[@birsingh5388]

@@LaravelDaily Thank you so much, I will check that out 👍

[@dev22221]

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

[@emekatimothyiloba699]

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?

[@LaravelDaily]

Because config caching wouldn't work this way if you need it

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 ?? [];

[@kaokao8981]

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.

[@ste8701]

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?

[@LaravelDaily]

That's your personal preference.

[@AndrewJudd]

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.

[@mohammadkhairi8014]

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

[@LaravelDaily]

Yes probably. But it depends on the situation.

[@adilizm704]

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

[@EmadHa]

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

[@PaulaBean]

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

[@jdrab]

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

[@PacificDev]

Nice one!

[@sajjadhossainshuvo6975]

Thanks a lot.

[@soniablanche5672]

why is the array being cast to object ?

[@99Spyder99]

Probably because he use the data in php, not outside, and he preffer calling it like $data->destination instead of $data['destination']

[@lynic-0091]

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.

[@gushung0]

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.

[@Neptun084]

🤔 does being a senior synonym of perfect?

[@Andris_Briedis]

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.

[@MichalKuzmicki]

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.

[@MoemenGaballah]

Very Good Thanks.

[@syracuse4612]

need a sequence of senior code reviews

[@helluci6449]

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

[@liquidsnake6879]

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

[@lynic-0091]

@@liquidsnake6879 you've seen worse? dayum

[@souravdutt7]

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

[@underflowexception]

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

[@arturpendrag0n270]

It should work fine with empty()

[@souravdutt7]

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

[@kenjohnsiosan9707]

very helpful.

[@JohnDoe-hs8np]

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 ) { ..._ 😉

[@kennethkipchumba2532]

This mistake can be very costly.

[@rorumets]

Very nice ❤

[@yonahernandez9073]

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?

[@LaravelDaily]

Semi-yes :)

[@ionelCristianLupu_]

There is still a duplicated exception message

[@LaravelDaily]

Good point, another potential improvement.

[@ikarolaborda726]

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

[@mzerone-g6m]

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

[@KirillHybrid-f4f]

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.

[@darwincabarrubias1368]

this is how students code at school

[@Jan-so5hm]

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

[@LaravelDaily]

That question is outside of the scope of this video, because I don't have more context about this project. It depends on the situation.

[@BlueJDev]

But making the request client side, eg using fetch API would expose the API key...

[@programmingmindset]

Senior developer with junior developer's flavour 🤪🤪🤪🤪

[@NikolaDjordjevic-ds6vn]

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

[@KG-id3hk]

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

[@cardboarddignity]

Seems like a "senior" developer that used to work on position I am right now. Really - the code style is absolutely the same :/

[@JouvaMoufette]

Won't lie. If this "senior" developer gave me this as part of a code exercise for a hiring process, I wouldn't hire them. I cannot believe they wrapped everything in a try/catch only for the catch to rethrow it. Just leave it unhandled at that point! In fact this is worse! Now you don't know which line threw the original message!

[@AndrewJudd]

Nor the real type of the exception, all you get is an error message which is pretty useless.

[@JouvaMoufette]

​@@AndrewJudd Yeah if something else threw a non-generic exception, now you lost the type of exception. This code was just gross all around, and reminds me of a project I was a part of that started as a Proof of Concept/working prototype, and weeks away from the deadline we were told that it needed to be production ready. So everyone had to rush and made bad decisions, including suppressing all exceptions thrown to be a generic "an error occurred" message in the logs.

[@AndrewJudd]

@@JouvaMoufette So very true

[@manu144x]

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.

[@ww-pw6di]

I've seen much worse code in production (PHP and others) and the reality is that all of that literal garbage works fine for their application. If it didn't it would have been replaced by a different pile of garbage or something better. "Shoulda coulda woulda" yes, but that's also what the customer will say about the budget for a piece of code that will never be viewed again.

[@frtrash766]

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.

[@ariumboroseno]

Seriously ?? Is that a senior version code ??? Seem like a junior version to me

[@maflones]

He has a point though. Laravel makes advanced programming available to people that have no clue what they are doing, which is most people that use Laravel, and thus we end up with a lot of shitty code. The framework compensates for this in a pretty good way, but still, dirty code all over the place.

[@DeTechDivus]

Whats more funny is that someone calls himself a senior and cry about his ugly code..

[@Daaboo]

I don't get a job because "lack of experience". But if this code is written by Senior developer, I'm over qualified.....

[@chengkangzai]

Hate to be the guy but ... BUGSNAG API KEY was expose 😅

[@LaravelDaily]

Yes I noticed but decided to not edit it out, as there's no point for anyone to use that key to log something into MY project where only ME has access to it :)

[@chengkangzai]

@@LaravelDaily i think it's still a problem which ppl can spam your bugsnag... Especially the bugsnag have limit of error record

[@stanislavkacmarik6210]

maybe I am little bit paranoid, but I always modify my private keys before every presentation :D And sometimes someone ask me, why its not working on his machine and then I figure out, that he copy pasted my modified private key, instead of following the documentation and create his own key.

[@grzesiekb9142]

You can kill somebody using hammer. Then the hammer is bad or person witch use this hammer? Hammer is tool === Laravel is tool.

[@underflowexception]

PHP and Laravel has a low barrier to entry so naturally it will attract a lot of bad programmers.

[@JouvaMoufette]

It's a very easy language to pickup, so it does at times tend to lead to developers that don't understand exactly what they're writing. But you can very easily get bad programmers in any language.

[@soniablanche5672]

You'd be surprised how many good programmers write cryptic and "cleaver" code just to save 1 or 2 lines

[@JouvaMoufette]

@@soniablanche5672 after as long as I've been developing professionally, it doesn't surprise me any more

[@upsator]

To be honest, except the api key there was nothing wrong with the code :) it was functional. Not readable ok, but functional. We dont know if it was written in hury. I saw much worst code from experienced developer just because it was easier and faster for him to write it that way just to have it done because he had a lot of other work.

[@sudeshryan8707]

Nobody in their right mind would have written 50 lines of spaghetti code in a HURRY where it's only needed 10 lines for same functionality. LOL

[@upsator]

@@sudeshryan8707 not so sure if you can imagine he would be like request than dd than something fail so if than dd and so on without thinking and this si the result without refactor :)

[@spicynoodle7419]

*! isset($json->rows) || $json->rows == []* could just become: *empty($json?->rows)*

[@Atiradeonvideo]

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

[@BernhardK91]

Blaming the framework for bad code is like blaming a hammer not working as a saw.

[@bw7868]

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.

[@mubasharrashid1201]

I strongly agree with you mate!

[@Novica89]

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

[@mubasharrashid1201]

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

[@mubasharrashid1201]

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

[@Novica89]

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

[@josibee]

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.

[@marcusgaius]

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.

[@ivanjelenic5627]

He probably didnt do it to keep the video short.

[@SodalisUK]

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.

[@vNYCblade]

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.

[@DimaSimonishvili]

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

[@EndreVarga_rnd]

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

[@DimaSimonishvili]

@@EndreVarga_rnd Haha there's always a room for an improvement.

[@Atz22]

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 ?

[@LaravelDaily]

I think it's related to cache, with this approach you can't cache config.

[@vanosgarage]

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]

@@vanosgarage thanks... May I know which server/PHP configuration where env() cannot be used?

[@michalgreksak]

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

[@NoOorZ24]

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

[@JouvaMoufette]

Yes! Then if there's a sandbox version of their api, you can point to that version easily

[@RamonRaa]

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

[@SaiyanJin85]

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!

[@LaravelDaily]

Yeah, good suggestion! I was actually thinking to use some collection operation instead of a longer foreach loop. But separate method also makes sense!

[@SaiyanJin85]

@@LaravelDaily Thanks!

[@nithinjoseph182]

why not use \Throwable instead of \Exception ?