Keep It Simple (KISS coding principle)

Tute Costa

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