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?