#6 I Will Alphabetize Your Case Statement
and other lessons in refactoring
This is going to be a trip through a minor refactor, with included code snippets. If you love/hate this format, leave a comment and tell me about it! And if you’d rather read it outside of your email inbox, click ‘Open in browser’ above.
“...make the change easy (warning: this may be hard), then make the easy change” - Kent Beck
A few weeks ago, my “easy change” was to add a conditional to a case statement. Here’s my journey through making that change easy.
The code chunk I was dealing with looked roughly like this:
def main_method(string_to_validate)
case validation
when validation_a then helper_a
when validation_c then helper_c
when validation_e then helper_e
when validation_b then helper_b
else "don’t know how to validate this"
end
end
private
def helper_c … end
def helper_b … end
def helper_a … end
def helper_e … endI wanted to add validation_d, and its corresponding helper.
What made that hard? The file was unorganized, and that disorganization brought up some questions: do I add my new case at the bottom of the list, the top, or somewhere in the middle? Do I try to group it with other cases that are like it? What about the helper function? Do I put that at the bottom of the file, or try to slot it in in the same order as I put the case in the case statement? When I go to write tests, where should they go within the test file?
We’re going to refactor the code, gradually, until the answers to all of those questions are obvious.
First question: where should I put the new case in the case statement? The answer: alphabetically1. But in order to put it in the case statement alphabetically, I had to alphabetize the rest of the case statement.
So after that commit, we have:
def main_method(string_to_validate)
case validation
when validation_a then helper_a
when validation_b then helper_b
when validation_c then helper_c
when validation_e then helper_e
else "don’t know how to validate this"
end
end
private
def helper_c … end
def helper_b … end
def helper_a … end
def helper_e … endNow, to solve my next point of indecision: where do I put the helper method for my new case? I’d like to put it in the order that it’s called, when looking at the file top-to-bottom2. Again, they’re currently out of order. So, before I add new functionality, I put the helper methods in order they’re called in the function.
We arrive at:
def main_method(string_to_validate)
case validation
when validation_a then helper_a
when validation_b then helper_b
when validation_c then helper_c
when validation_e then helper_e
else "don’t know how to validate this"
end
end
private
def helper_a … end
def helper_b … end
def helper_c … end
def helper_e … endGreat! It’s now clear (to me), where my new case statement should live, and where its helper is going in the file. Two commits down (both non-functional changes), two more to go.
Next up, the test files! I know I want to test my new case in the case statement, and the same “where in the file do I put this” questions will come up. So, I give the test file a glance and find that it’s already in order, and everything is ready to go! Just kidding. It is already in order, but only because roughly a third of the case statements are tested, and they randomly happened to be in order.
My next commit is to backfill a set of tests for every case in the case statement that currently exists. This takes a bit of time, but it’s for a good cause, especially given what comes next.
While writing tests, I discovered a small logic bug in one of the untested functions!
This function takes in a value and returns an error string if the validation fails. This is the function as it was written when I wrote the tests:
def validate_string_number_is_positive(value)
return unless value.is_a?(String)
if /\A\d*\z/.match?(value)
‘String is not numeric.’
end
if value.to_i < 0
‘String value is negative’
end
end
# test
describe ‘StringNumberIsPositive’ do
it ‘validates correctly’ do
expect(validate(nil)).to be_empty
expect(validate(‘12345’)).to be_empty
expect(validate(‘-12345’)).to eq(‘String value is negative’)
expect(validate(‘12345ABC’)).to eq(‘String is not numeric’)
end
endWhat happened? The first three expectations passed, but the last one failed, in this way:
expect(validate(‘12345ABC’)).to eq(‘String is not numeric’)
# failed, returned nilLooking at some of the rest of the helpers, it was clear the root of this problem was just a copy-paste oversight, paired with Ruby’s implicit return value. What the code meant to say, was this:
def validate_string_number_is_positive(value)
return unless value.is_a?(String)
if /\A\d*\z/.match?(value)
return ‘String is not numeric.’
end
if value.to_i < 0
return ‘String value is negative’
end
endBut that brought up new test failures! Now, the third expectation started failing:
expect(validate(‘-12345’)).to eq(‘String value is negative’)
# failed, return ‘String is not numeric’That makes sense: the ‘-’ in the string doesn’t match the regex that’s checking whether or not the string’s numeric. So now, negative numbers are returning with the “wrong” error message.
Easy enough fix; swap the if blocks.
def validate_string_number_is_positive(value)
return unless value.is_a?(String)
if value.to_i < 0
return ‘String value is negative’
end
if /\A\d*\z/.match?(value)
return ‘String is not numeric.’
end
endNow, all four expectations succeed as written:
describe ‘StringNumberIsPositive’ do
it ‘validates correctly’ do
expect(validate(nil)).to be_empty
expect(validate(‘12345’)).to be_empty
expect(validate(‘-12345’)).to eq(‘String value is negative’)
expect(validate(‘12345ABC’)).to eq(‘String is not numeric’)
end
endIs it the most comprehensive code ever written? No, but now at least it does what we expect it to do, which is pretty good for a test-backfill-in-preparation-for-new-functionality.
Backfilling the tests is one commit, fixing the functionality is another. And those, added to the two commits from earlier, went in a PR together before working on the new feature. Then, the actual change I had set out to make all along was easy, straightforward, and frictionless.
Finally, we add our extra case and function:
def main_method(string_to_validate)
case validation
when validation_a then helper_a
when validation_b then helper_b
when validation_c then helper_c
when validation_d then helper_d
when validation_e then helper_e
else "don’t know how to validate this"
end
end
private
def helper_a … end
def helper_b … end
def helper_c … end
def helper_d … end
def helper_e … endIt’s not hard to do things this way, but it does take some attention and some willingness to put in a small amount of extra work to make life easier a bit later.
There are two takeaways here. One is that I care a lot about doing my job well. I’ll take the time to refactor things, backfill missing tests, fixup small things, and then make my easy change. The other is that I’m a very lazy developer. I don’t want to have to make any extra decisions while programming that aren’t strictly necessary, and that means that I often have to take an extra step or two to refactor and remove the necessity of a future decision. But by doing so, I get benefits like knowing where to put my code, being able to rely on tests to tell me whether my code is correct or not, and the minor moment of calm + clarity as I put things in order.
As I said to a coworker: “I know things are going to get all out-of-order when someone needs to add more functionality to this file, and that’s okay”. She said “yeah, but at least, for right now, everything is in its place.”
Some good things:
I learned about the fun scale while having some solid type 2 fun at work last week
We finally finished watching Halt and Catch Fire and it was very much worth it: expertly paced until the very end
This was a great addition to the dinner menu this week
Drifting Off with Joe Pera is our new evening routine
Capybaras are comfy in hot springs (and scientists can prove it)
A list of the wildest thing that happens in every Animorphs book
Until next time; take care of yourselves
I’m sure there are other heuristics to use when ordering case statements, but alphabetically will bring the right amount of order to chaos 99% of the time
Some would argue this is entirely unnecessary, because everybody has ctrl+f, if not go-to-definition built into their IDE. I like it, because it helps me make sense of helper-function-mess. And if everybody else is already using ctrl+f, they won’t care whether things are in my preferred order vs. a different order! So there.