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:

 1public static function before($subject, $search)
 2{
 3    if ($search == '') {
 4        return $subject;
 5    }
 6 
 7 
 8    $pos = strpos($subject, $search);
 9 
10    if ($pos === false) {
11        return $subject;
12    }
13 
14    return substr($subject, 0, $pos);
15}

And the "refactored" code:

1public static function before($subject, $search)
2{
3    return empty($search) ? $subject : explode($search, $subject)[0];
4}

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 great work!

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.

Find this interesting? Let's continue the conversation on Twitter.