Flatten must be banned
I have strong opinion about some patterns in Ruby. Certain things are frequently
used in a terribly wrong way (what a surprise). In this post I will explain what
is wrong with how Array#flatten is used sometimes.
Array#flatten
Argument-less flatten should not exist. In my experience, only in very rare
cases do we need to call it. Most often, flat_map should be used instead.
In most of the remaining cases, flatten(1) is an option.
flatten without arguments means that you have no idea what kind of input to
expect. This is a violation of YAGNI principle and just sloppy coding.
There are literally hundreds of examples of abusing flatten on GitHub. Let’s
review some of them.
Case One
# bad
reloadable_paths.map(&:existent).flatten
# good
reloadable_paths.flat_map(&:existent)a.map{}.flatten is not always equivalent to a.flat_map{}, but in this particular
case it is, as well as in many similar cases.
Case Two
# bad
html.scan(/.../).flatten.each do |anchor|
...
# good
html.scan(/.../) do |(anchor)|
...Way simpler, right?
Case Three
# bad
def helper_attr(*attrs)
attrs.flatten.each#...
end
# good
def helper_attr(*attrs)
attrs.each#...
endThis pattern is especially popular among Ruby developers. With this pattern, the
caller is allowed to call helper_attr in many different ways:
HELPERS = %i[current_account current_user current_role]
helper_attr :current_user
helper_attr HELPERS
helper_attr [[:bullshit]], [[[[[:look_what_i_can_do]], [:lol]]]], :okSome people call it “convenient”. I respectfully disagree. I consider this code
unconfident, vague and sloppy. Interface of helper_attr is overly broad.
Without losing much readability, flatten can be dropped and the method will
become more strict, it will obtain a well-defined type, if you wish:
HELPERS = %i[current_account current_user current_role]
helper_attr :current_user
helper_attr(*HELPERS)
# the following is not considered a valid input
helper_attr [[:bullshit]], [[[[[:look_what_i_can_do]], [:lol]]]], :okIf the splat in helper_attr(*HELPERS) really, really, really bugs you, then
there still exists an option way better than argument-less flatten: flatten(1).
def helper_attr(*attrs)
attrs.flatten(1).each#...
end
helper_attr :current_user
helper_attr HELPERS
# the following is not considered a valid input
helper_attr [[:bullshit]], [[[[[:look_what_i_can_do]], [:lol]]]], :okCase Four
# bad
[forwarded_ips, client_ips, remote_addr].flatten.compact
# good
[*forwarded_ips, *client_ips, remote_addr].compactIf we know the shape of the input, why on Earth do we need to use the overly generic and slow method that hides our intent instead of making it as clear as possible?
Case Five
# bad
[token].flatten.first
# still bad, but better
Array(token).first
# or
Array.wrap(token).firstSurprisingly often I see this [something].flatten code. In most cases, what
people want is: if it’s a collection, keep it as is, otherwise wrap it in array.
This is exactly what Kernel#Array does.
Conclusion
Do I need to say that flatten-less code is generally faster than equivalent code full
of argumentless flattens?
But mind you, performance is not my main concern. My main concern is
maintainability. I consider naked flatten a code smell.
In many cases code with flatten is the opposite of what I would call
Confident Ruby.