Week 28
Phew, another busy week working on my main project! If you've been following along, you'll know I've been working on an account purging project which utilises long-running processes. I've had to react to unexpected issues that have cropped up along the way: I've had to think on my feet and come up with some creative approaches to the problems! In doing so, I've been reminded again about the importance of code readability: when a process is running over several days, is churning through large amounts of data per second, and you discover a problem, you need to react fast... time is of the essence and if a solution can only be reached by reading and understanding code, then you'll be all the more relieved that you and your team spent those extra few moments thinking about how to make the code readable... you'll be glad that you thought about how it will read next week, next month, next year... that you thought about how someone else, without all the context you have, will read and make sense of it.
Source code will be read potentially many times but written only once...
Software Productivity Consortium (SPC), many moons ago
Robert C. Martin expressed the very same truth in Clean Code and if you've been working with software for any length of time and you've had to read and slowly re-read and then re-read again a line of code to understand it... you'll know it for yourself.
In fact, I'd go as far as saying if code were written only once, as the quote suggests, we'd be in a slightly better position! But we all know that that's not the case... code is written, amended, changed, refactored and then totally rewritten and the loop continues... add to this the fact that each new developer touching the code, for better or for worse, brings their own ideas and way of expressing themselves to bear on the code. Sure, linting can enforce formatting and code style but can we enforce consistency in terms of the expression of ideas and concepts?
A common problem I often encounter are methods with a boolean parameter. Sometimes referred to as a "flag", it's the practice of varying the behaviour of a method based on a TRUE/FALSE argument. The boolean parameter may or may not have a default value... consider this very simple example interface:
public function getMemberships($expired = FALSE): array;
It's not hard to imagine that $expired might be used to conditionally add a clause to SQL query or alter in some other way what is returned in the array...
It's also not hard to reach the conclusion, even without comments or tests, that calling the method without arguments (or with FALSE) should return an array of active membership data and that calling it with TRUE should return an array of expired membership data. But what if passing TRUE means include expired membership data? In really life, as we know, it gets much more confusing than this, even when there are comments and tests to help us out!
Furthermore, I'm one of those people who find it hard to parse boolean negation and let's, for simplicity sake, agree $expired is a boolean and not muddy the water with truthy values in the following examples:
if (!$expired) {
// We only end up here if $expired === FALSE, parse time 60+ seconds.
}
I find the parse time increases when the boolean variable isn't named helpfully. I personally find it much easier to parse:
if ($expired === FALSE) {
// We only end up here if $expired === FALSE, parse time 1 second.
}
and let me hit you with this improvement:
if (FALSE === $expired) {
// We only end up here if $expired === FALSE, parse time 1 second.
}
Okay, I'm getting controversial but it's worth considering these approaches amongst your teams and settling on what you find most readable.
Getting back to the method itself, though... it is not only confusing to the reader, it breaks the Single Responsibility Principle: this method clearly does more than one thing.
An obvious alternative approach would be to define the following interface:
public function getActiveMemberships(): array; public function getExpiredMemberships(): array;
And if you need it:
public function getAllMemberships(): array;
Even without comments, it's clear what the responsibility of each of these methods is, it also highlights the importance of thoughtfully naming your methods, variables, classes, interfaces and so on...
We could go further. We've type-hinted the method return type, it must return an array. That's a start, it does make the code more readable but it ultimately puts the onus on the calling code to know about the structure of the array being returned. Sure, we can document and annotate our code which further helps with readability (and can also prevent some classes of bugs when combined with the use of static analysis tools such as PHPStan and Psalm) but perhaps we could return something better...
public function getActiveMemberships(): MembershipCollectionInterface;
Now our method must return an instance of a class implementing MembershipCollectionInterface (which could even extend Iterator) and calling code can be confident about what methods are available to call.
Yes, I hear you. It's easy to return an array! Why go to all that trouble of creating interfaces? Well, I like certainty and predictability.
As with everything, there are exceptions to the rule. Perhaps you can think of scenarios where using a boolean parameter as a flag is an acceptable approach... perhaps there are some situations where an array is best... perhaps you've worked on projects where type-hinting has been used heavily and it caused problems... as always, do let me know on Twitter!
Outside the developer cabin
Training for the Bob Graham Round continues... I helped Jeff Pelletier on section five of his round and even took a turn with the GoPro... his short film gives you a good idea of what it's all about and those with a keen eye might be able to spot me.
I also did an out and back from Wasdale Head to Scafell Pike, the highest and most prominent mountain in England. Unfortunately it was another bad weather day and I spent most of the day in cloud with limited visibility. It was my first time in this area of the Lake District and I had picked a direct route (not the tourist path) up to Scafell, which is the last summit on the section and then over to Scafell Pike, the second to last summit on the section, via something called the West Wall Traverse. What I should have done was my homework... down-climbing the West Wall Traverse in the wet is no joke and is quite honestly something I never want to do again! Fortunately climbing back up it in the direction I'll be going when I do the round is much easier and nowhere near as scary.