Keep It Simple (KISS coding principle)
The app we maintain has a set of steps (currently two) a user may have to fill before moving on. One step is always shown, while the other can be disabled with a feature flag. We assume this list may grow, and even when it doesn’t, we know code is copied over to other parts of the codebase, so we better get it right early.
Our first implementation:
1
2
3
4
5
6
def supported_steps
{
current_medications: true,
allergies_review: Feature.enabled?(PREVISIT_ALLERGIES),
}.select { |_step_name, enabled| enabled }.keys
end
A suggestion during code review:
1
2
3
4
5
def supported_steps
steps = [:current_medications]
steps << :allergies_review if Feature.enabled?(PREVISIT_ALLERGIES)
steps
end
It’s quicker to get what’s happening in the second option, and while the methods stay short, they are practically equal. Our code reviews can get lengthy so we try not to chime in when either option is fine, but I suggested the first as a simpler option (even if not easier)[1]. Let’s see why.
The first approach starts with a hash with step names as keys and statuses as values. It ends by selecting enabled steps calling two methods on the anonymous hash. It’s declarative: a quick glance tells what steps we have, and when would those be enabled.
To add a step, one adds a line following the step-name/enabled pattern. We can rely on the methods at the end doing their thing. We deal with the what (the values) more than with the how (the computations to get there).
It requires we know the hash structure and what select(k, v) and keys
methods do.
The suggested version starts by creating a local variable with an array with
a single element, that’s unconditionally there. It then mutates the array to
contain another element, when the if condition holds true. It ends by
returning the possibly-mutated array.
To add a step, one adds another line that mutates the array, which has a name at the left and the condition at the right (much alike with the first approach).
It requires we know the array, a variable assignment, the shovel operator, and
the conditional if. We know those since our first steps with computers, and in
that sense, it’s easier.
But now it’s more verbose, and we construct the result line by line. Alongside
the step name and the condition, each line will contain the steps variable and
the shovel operator. It may also contain the if call, in which case it
negatively affects the cyclomatic complexity of the method (flog pain-score
goes up, CodeClimate GPA goes down). We are forced to think about the how
whenever we think of the what. The implementation is mixed in with data in an
imperative way. The how and the what are intertwined together, making it
more complicated.
Thinking twice before adding any local or instance variable helps me keep my code simpler.
PS: Here’s Mauro’s suggestion to make that code even more readable (extracts an intention revealing method).
1
2
3
4
5
6
7
8
9
10
def enabled_steps
supported_steps.select { |_step_name, enabled| enabled }.keys
end
def supported_steps
{
current_medications: true,
allergies_review: Feature.enabled?(PREVISIT_ALLERGIES)
}
end
[1] See Rich Hickey’s talk, Simple Made Easy