Functional question: what is this line intended to calculate? It could return 0 for age < bucket_size, which conflicts with the definition of bucket_number_list as starting at 1.
A general rule I find useful: if you see multiple functions with similar prefixes / suffixes, consider if there’s a data structure hiding in the code.
A similar outcome from a different thing: if you see multiple arguments that are always handled together, consider if there’s a data structure hiding in the code.
For instance, a struct called Buckets:
defmodule Buckets do
defstruct [:size, :start, :count]
def index_of(x, b) do
(x - b.start + b.size) / b.size
|> floor()
|> max(0)
|> min(b.count+1)
end
def label_for(0, b), do: "under #{upper_bound(0, b)}"
def label_for(index, %{count: count} = b) when index == count + 1, do: "#{lower_bound(count, b)}+"
def label_for(index, b) do
"#{lower_bound(index, b)}-#{upper_bound(index, b)}"
end
def lower_bound(0, _), do: nil
def lower_bound(index, b), do: b.start + (index - 1) * b.size
def upper_bound(index, %{count: count}) when index == count + 1, do: nil
def upper_bound(index, b), do: b.start + index * b.size
def trim_ends(map, b) do
map
|> trim_at(0)
|> trim_at(b.count + 1)
end
defp trim_at(map, index) do
if Map.get(map, index) == 0 do
Map.delete(map, index)
else
map
end
end
def zero_pad(map, b) do
1..b.count
|> Enum.reduce(map, fn index, acc ->
Map.put_new(acc, index, 0)
end)
end
end
Then the main function can use these higher-level concepts:
defp histogram_frequencies(responses, variable, bucket_size, start_value, bucket_count) do
# could also be passed in as an argument
buckets = %Buckets{size: bucket_size, start: start_value, count: bucket_count}
responses
|> Enum.frequencies_by(fn %{^variable => value} -> Buckets.index_of(value, buckets) end)
|> Buckets.trim_ends(buckets)
|> Buckets.zero_pad(buckets)
end
There are a lot of advantages to this approach:
- the small functions in
Bucketsare easier to read / test / debug - naming gets easier - inside
Buckets, just sayingcountis sufficient (versusbucket_count) - errors like swapping
start_valueandbucket_size- which will run, since both are numbers, but produce nonsense output - are avoided by passing around a whole struct
One other side-effect of this approach: for large lists in responses where at least one value lands in the “over the limit” bucket, this method will be about twice as fast because it only constructs the frequencies map once!
