Take those pipelines and make them their own defp functions; the case should probably be an “if”.

There’s general elixir coding guidelines too. Fetch vs fetch! vs get have specific meanings, avoid is_ functions unless they are guards,

Avoid variables called “value”, lol. I have a linter for that to get me out of the habit.

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 Buckets are easier to read / test / debug
• naming gets easier - inside Buckets, just saying count is sufficient (versus bucket_count)
• errors like swapping start_value and bucket_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!

Regarding efficiency and performance, you could improve it by doing some of these successive Enum operations in one pass, which should avoid building intermediate lists and walk them twice.

filter |> map could be re-implemented with a comprehension:

  |> Enum.filter(fn {key, _value} -> key >= end_bucket end)
  |> Enum.map(fn {_age, count} -> count end)

could be

for {age, _count} <- age_frequencies, age >= end_bucket, do: count

map |> map should typically be avoided, since you can do it directly in one pass:

  |> Enum.map(fn {_key, value} -> Enum.map(value, fn {_x, e} -> e end) end)
  |> Enum.map(fn p -> Enum.sum(p) end)

could be

  |> Enum.map(fn {_key, value} -> Enum.map(value, fn {_x, e} -> e end) |> Enum.sum() end)

and even map |> sum could be replaced by Enum/reduce/3 here (although this one might be slightly less readable):

  |> Enum.map(fn {_key, value} -> Enum.reduce(value, 0, fn {_x, e}, acc -> e + acc end) end)

Credo has some checks like MapMap, FilterFilter, MapJoin… to help detect some of these patterns.

I didn’t mention Stream, since it also comes with some overhead and would probably not improve performance here except if you are working with large lists.

Just a minor note, by convention, the functions in a module for a struct should take the struct as a first argument, if that makes sense. Or generally, not just for structs, if the module is a noun, (List, String, Registry, etc)

-without reading the whole thread-

When those are private functions, I would take the pragmatic ordering that makes the flow in the public function as clear as possible. After all

the code (convention red.) is more what you’d call “guidelines” than actual rules. - Captain Barbossa

Public functions however should be predictable; taking the struct as first argument.

edit: Barbossa notified me the guidelines only apply when you are a pirate. Sorry for the confusion.