Idiomatic style, or taking DRY too far?

Support
1

I recently had code review feedback on a piece of code that I didn’t expect. The feedback is based on a perspective with a couple of good points, but among a couple of different perspectives with their own good points.

(DRY = Don’t Repeat Yourself)

I am here seeking further opinions, perspectives, and/or logical reasons for choosing any implementation style over another, among the following implementations.

Setup:

open class A {
    fun doThing() {}
}
class B : A()
class C: A()

val foo = A()
val bar = B()
val baz = C()

Now, I want to invoke doThing() on foo, bar, and baz. There are plenty of ways to do this. Here are three approaches:

  1. Straightforward approach:
foo.doThing()
bar.doThing()
baz.doThing()
  1. DRY approach
listOf(foo, bar, baz).forEach { it.doThing() }
  1. Declarative DRY approach
inline fun <T> forEach(vararg items: T, action: (T) -> Unit) {
    for (element in items) action(element)
}

forEach(foo, bar, baz) { it.doThing() }

Here is what I see so far:

  1. Straightforward
  • Pros
    ** No excess constructs. Easy to read when your operation is a single function invocation.
  • Cons
    ** Repetitive (violates DRY). Code evolution could get hairy if the repeated routine becomes more complex than a single function invocation.
  1. DRY
  • Pros
    ** No hand written code repetition (no copy/pasting code). Altering the repeated routine for one item will give the same altered routine to the other items without fail. Enclosing the repeated routine in a lambda is good if the routine ever grows more complex.
  • Cons
    ** Less efficient execution than straightforward approach
    ** declares a list, when having a list is not what we are actually interested in.
  1. Declarative DRY
  • Pros
    ** Same pros as DRY approach
    ** Declares the exact intent, without excess declaration of a random list just so we can find our way to a forEach.
  • Cons
    ** Less efficient execution than straightforward approach
    ** Not in the standard library, and so unfamiliar. Initial readability may suffer

Now I request the thoughts of the public:

  • Is there a reason you would reject any of the above approaches almost universally?
  • Or perhaps would you decide on an approach strictly on a case by case basis, measuring all pros and cons?
  • Or would you generally lean towards one or two approaches over the others (maybe based on more than just immediate needs), only falling back to your lesser-preferred when the cons significantly stand out?

Circumstantial variables to keep in mind:

  • What happens when the list of items we want to affect grows? foo, bar, baz, buz, beep, boop, so on, so forth
  • What happens when the routine we want to perform grows more complex? e.g. instead of a single function invocation, perhaps a very complex routine written in place
  • What happens when neither of the above are true, and the list stays short, or the routine stays simple?
2

If you only ever have the three things, I think #1 is just fine. Don’t overthink it. I might do #2 myself, just because it’s marginally clearer that you’re indeed doing the exact same thing on each element. #3 seems overkill to me. If you’re worried about efficiency with #2 re: the list allocation, you could also say sequenceOf instead, or I suppose arrayOf. But definitely premature optimization.

DRY, to me, is less about repeating something as trivial as invoking the same method, and more about repeating entire regions of code – if you’ve copy/pasted and then tweaked a 10-20 line method in 3 different places, there’s a real risk of making bug fixes in one version and forgetting to port it over to the other ones. If you’re talking about calling a method on three objects, it’s not even in the same ballpark.

3

IMHO, the first aspect of code to prioritize is readability (how easy it is to understand). This applies to the DRY principle as well.

Option #1 is more readable to me when the list is small (less than 5-10 items depending on the length of the instance names and method name/params)

For this reason, I would go with #1 until the list grew to more than 5 items or the operation grew in complexity.

If the operation on each item grew in complexity (maybe two function calls per item, a complex call, or a call with named params), I would probably stick with #1 but make sure it fits into a single method call or maaaybe an apply{...} block (but if it can be in an apply bock, it could also just be wrapped into another function).

If the list grew past 5 items, I would go with option #2 but maybe with each item indented on their own line (depending on if the list contents are intended to be known by the coder)

listOf(
    foo,
    bar,
    baz,
    buz,
    beep,
    boop,
).forEach { worker ->
    worker.doThing()
    ...
}

Option #3 is easily understood even though it’s just a wrapper for listOf(...).forEach {...}. Personally, I can’t say I’d use it or not since, despite being more concise, it doesn’t improve the code’s readability (IMO) when there are many items. Not something I’d object to in a code review–I’d probably support it since it’s very clean.

Now, assuming the list became long or the operation got complex, I’d personally put the code in a function for the purpose of isolating it and naming it.

...
doSomethings() // Defines the list within the function.
// or
doSomethings( // Takes in the list as a varargs parameter.
    foo,
    bar,
    ...
)