There was a good discussion on Twitter yesterday regarding a code contribution to the Laravel framework. It ended with some good questions about the distinctions between “refactoring” vs “changing” code.
While I want to focus on these distinctions, let’s first focus on the code change.
Here’s the original code:
public static function before($subject, $search)
{
if ($search == '') {
return $subject;
}
$pos = strpos($subject, $search);
if ($pos === false) {
return $subject;
}
return substr($subject, 0, $pos);
}
And the “refactored” code:
public static function before($subject, $search)
{
return empty($search) ? $subject : explode($search, $subject)[0];
}
A nice, clean one liner. All tests passed and the code was merged.
Developers with a keen testing eye may have already noticed an issue, but most noticed I quoted refactored.
That’s because this code wasn’t refactored, it was changed. Let’s recall the definition of “refactor”.
to restructure software without changing its observable behavior
In this case, because the new code behaves differently than the original, the observable behavior changed.
How does it behave differently?
This takes that keen testing eye, but a ready example is when $search is 0. The original code would search within $subject and return the string before the 0 occurrence. Whereas the new code would return early with $subject. Not the same behavior.
Unfortunately the existing tests did not catch this. As such, it was on the contributor to spot this changed behavior - which they later did and submitted a patch with the missing test. Upon doing so, this became a true refactor and nice contribution!
However, this lead to another interesting question - since all the existing tests passed, was the original contribution a successful refactor.
Given the symbiotic relationship between refactoring and testing, some consider the tests to be the requirements. So if all tests pass, you met the requirements.
I think that’s a slippery slope. For me, the definition of “refactoring” again provides the answer through its own question - did we change the observable behavior?
In this case, yes - we can observe the behavior changed. So the original contribution was not a true refactor, despite the passing tests.
Nonetheless, I think there are some other interesting points around refactoring and testing. Ones I will explore in a future post. For now, be mindful you’re truly “refactoring” code and not “changing” code.
Top comments (8)
Refactoring should also aim to make something either 1) more readable, 2) more performant (because of an actual need to make it more performant), or 3) more flexible to changes that have happened in the past. The code change above fails all 3. It's harder to understand what's happening thus making it less flexible to changes that may be needed in the future.
There is nothing special about a 1 liner (it's what I call "clever coding" always with the quotes, and it's a really bad practice). A computer can read 10 lines really really quickly, and it generally takes as long or longer for a human to decipher what the 1 liner actually does vs the 10 line equivalent.
I couldn't agree more about one-liners. Also using "tricks", little-known features and other "clever" stuff usually just makes the code harder to comprehend. IMHO the code example above doesn't really need refactoring anyway.
Great comment, some people still/will continue to think that "one line" is a huge performance optimization
If its against a test suite, its refactoring full-stop. Insufficient tests are a separate concern.
That's the "gray-area" I want to discuss in my follow up post. 😉
I will read OFC,
I think for me the iterative nature of anything we want to do means having crap tests (consistency first) is a more build with foundations approach for those starting out or unsure what the tests points should be.
When I started to code in the 90's nobody showed me tests; nobody paying me asked for them. It was mid 00's before I realized I had to retro-fit my skills with an essential component.
Of course there's always room for improvement red->green->refactor, people over processes, but the kernel of development has to be testing something, not necessarily testing the right thing. Finding that right thing takes
time and experience. Even then we all often get it wrong 😉
Interesting point.
IMO it's the only responsibility of the contributor to check if the changed code is sufficiently covered by tests to make his change a refactoring. This is true regardless if he is the only contributor in the project or one of many.
It's like being careful when crossing a street even if the lights are green.
I've definitely done this in the past. I'd too excited to after "refactor"-ing a method to a one-liner and overlooked the fact that I've made that method destructive. Great post!