Refactoring - is it appropriate to simply rewrite code, as long as all tests pass?

Question

I recently watched "All the Little Things" from RailsConf 2014. During this talk, Sandi Metz refactors a function that includes a large nested if-statement:

def tick
    if @name != 'Aged Brie' && @name != 'Backstage passes to a TAFKAL80ETC concert'
        if @quality > 0
            if @name != 'Sulfuras, Hand of Ragnaros'
                @quality -= 1
            end
        end
    else
        ...
    end
    ...
end

The first step is to break the function into several smaller ones:

def tick
    case name
    when 'Aged Brie'
        return brie_tick
    ...
    end
end

def brie_tick
    @days_remaining -= 1
    return if quality >= 50

    @quality += 1
    @quality += 1 if @days_remaining <= 0
end

What I found interesting was the way these smaller functions were written. brie_tick, for example, was not written by extracting the relevant parts of the original tick function, but from scratch by referring to the test_brie_* unit tests. Once all of these unit tests passed, brie_tick was considered done. Once all of the small functions were done, the original monolithic tick function was deleted.

Unfortunately, the presenter seemed unaware that this approach led to three of the four *_tick functions being wrong (and the other was empty!). There are edge cases in which the behaviour of the *_tick functions differs from that of the original tick function. For example, @days_remaining <= 0 in brie_tick should be < 0 - so brie_tick does not work correctly when called with days_remaining == 1 and quality < 50.

What has gone wrong here? Is this a failure of testing - because there were no tests for these particular edge cases? Or a failure of refactoring - because the code should have been transformed step-by-step rather than rewritten from scratch?

Answer 1

Is this a failure of testing - because there were no tests for these particular edge cases? Or a failure of refactoring - because the code should have been transformed step-by-step rather than rewritten from scratch?

Both. Refactoring using only the standard steps from Fowlers original book is definitely less error prone than doing a rewrite, so it is often preferable to use only these kinds of baby steps. Even if there are no unit tests for every edge case, and even if the environment does not provide automatic refactorings, a single code change like "introduce explaining variable" or "extract function" has a much smaller chance to change the behaviour details of the existing code than a complete rewrite of a function.

Sometimes, however, rewriting a section of code it is what you need or want to do. And if thats the case, you need better tests.

Note that even when using a refactoring tool, there is always a certain risk of introducing errors when you change code, regardless of applying smaller or bigger steps. That's why refactoring always needs tests. Note also that tests can only reduce the probability of bugs, but never prove their absence - nevertheless using techniques like looking at the code and branch coverage can give you a high confidence level, and in case of a rewrite of a code section, it is often worth to apply such techniques.

Answer 2

What has gone wrong here? Is this a failure of testing - because there were no tests for these particular edge cases? Or a failure of refactoring - because the code should have been transformed step-by-step rather than rewritten from scratch?

One of the things that is really hard about working with legacy code: acquiring a complete understanding of the current behavior.

Legacy code without tests that constrain all of the behaviors is a common pattern in the wild. Which leaves you with a guess: does that mean that the unconstrained behaviors are free variables? or requirements that are underspecified?

From the talk:

Now this is real refactoring according to the definition of refactoring; I'm going to refactor this code. I'm going to change its arrangement without altering its behavior.

This is the more conservative approach; if the requirements may be underspecified, if the tests do not capture all of the existing logic, then you have to be very very careful about how you proceed.

For certain, you can assert that if the tests are inadequately describing the behavior of the system, that you have a "failure of testing". And I think that's fair -- but not actually useful; this is a common problem to have in the wild.

Or a failure of refactoring - because the code should have been transformed step-by-step rather than rewritten from scratch?

The issue isn't quite that the transformations should have been step-by-step; but rather that the choice of refactoring tool (human keyboard operator? rather than guided automation) wasn't well aligned with the test coverage, because of the higher error rate.

This could have been addressed either by using refactoring tools with higher reliability or by introducing a wider battery of tests to improve the constraints on the system.

So I think your conjunction is poorly chosen; AND not OR.

Answer 3

Refactoring should not change the externally visible behaviour of your code. That’s the goal.

If your unit tests fail, that indicates you changed the behaviour. But passing unit tests is never the goal. It helps more or less to achieve your goal. If refactoring changes the externally visible behaviour, and all unit tests pass, then your refactoring failed.

Working unit tests in this case only give you the wrong feeling of success. But what went wrong? Two things : The refactoring was careless, and the unit tests were not very good.

Answer 4

If you define "correct" to be "the tests pass", then by definition it is not wrong to change untested behaviour.

If a particular edge behaviour should be defined, add a test for it, if not, then it's OK to not care what happens. If you are really pedantic, you can write a test that checks true when in that edge case to document that you don't care what the behaviour is.