Geni Jaho3 min read
Take a look at this little method below. Three issues come to mind the first time you see it. Unnecessary calls to
array_key_exists, long chaining of
if conditions, and a redundant nested code. Let's see how it can be improved, without changing its behavior.
The first thing we want to do is get rid of all those calls to
array_key_exists. The reason for that is that it can be written much more simply. The null coalescing operator is meant for exactly the same purpose we have here, to be safe from exceptions of missing array keys.
The code basically wants to retrieve a
$cityName value from the
$addressArray, which has many keys to represent the name of a city. It traverses the array keys one by one until it finds a non-empty value. It can be made shorter, however, by replacing the
if conditions with the ternary operator
?:. The ternary operator will return the left-hand value if it evaluates as
true, otherwise, it will return the right-hand side.
At the end of these empty value checks, the code has one more condition. If the value is still empty, it sets
$cityName = 'error';. Then, only if this value is not
'error', it allows the "happy path", which is fetching or creating the city, launching an event, and returning that
Now either the programmer wanted to make sure not to proceed with the happy path if they got an empty value, or they wanted to prevent getting a value of
'error' from the
$addressArray itself, which is unlikely. Taking into account how the rest of the code is using this method, it's safe to say that they simply wanted to avoid empty values. In that case, we just check for empty values.
The code above looks good, but it can look a little bit better. That
if condition is visually pushing the main bulk of code on the right of the screen, although by a very small margin. But what if the main code was longer, and more complex?
This happens pretty commonly, checking for the right conditions to execute the happy path, and if those conditions aren't met then return an error value. To fix this, you should "exit early", which means you test for the error condition (or exit condition) first. If that condition is met, exit right away, and the rest of the code can run without being nested.
And that's it. These little changes mostly help for making the code more readable, and a little bit shorter.
The code used for illustration is taken from the OpenLitterMap project. They're doing a great job creating the world's most advanced open database on litter, brands & plastic pollution. The project is open-sourced and would love your contributions, both as users and developers.