coen.bakker

coen.bakker

Beyond code that gets it done: code readability. Any advice?

Besides learning Elixir and ‘getting things done’ with it, I hope to also learn how to write code that is highly readable – and therefore also easy to grasp. I still have a lot to learn in that regard, I’m sure. I try not to rush to a new problem to write a solution for and first clean up some of the mess I have left behind. The code might work completely, but it’s never fun to having to come back to my own code a while later and feeling tempted to rewrite the code, rather than to understand what I wrote in the first place because of the mess.

I have noticed that I often find Elixir code more readable than JavaScript code (given being familiar with both). The pipeline pattern in particular. Nevertheless, I am interested in any of your personal advice about writing readable code.

To give you an example. I wrote this function today. I didn’t think it would be this long and involved. I want to clean it up further (I made a tiny start already).

I love pipelines that consists of highly descriptive code. However, in this case that would mean writing a lot of helper function, possibly.

I have also seen people put comments behind pipe elements, to provide description. Often, that results in a lot of info located in one place, though.

  alias AppWeb.Component.Helpers

  defp histogram_frequencies(responses, variable, bucket_size, start_value, bucket_count) do
    end_bucket = start_value + bucket_count * bucket_size
    max_value =
      responses
      |> Enum.map(fn %{^variable => value} -> value end)
      |> Enum.max

    bucket_number_list = Enum.to_list(1..bucket_count)
    zero_list = Enum.map(bucket_number_list, fn b -> {b, 0} end)
    sums =
      responses
      |> Helpers.frequencies(variable)
      |> Enum.filter(fn {age, _count} -> age < end_bucket end)
      |> Enum.map(fn {age, count} -> {trunc(age/bucket_size), count} end)
      |> Kernel.++(zero_list)
      |> Enum.group_by(fn {key, _value} -> key end)
      |> Enum.map(fn {_key, value} -> Enum.map(value, fn {_x, e} -> e end) end)
      |> Enum.map(fn p -> Enum.sum(p) end)

    lower_limits_buckets = Helpers.lower_limits_buckets(bucket_size, start_value, bucket_count)
    upper_limits_buckets = Helpers.upper_limits_buckets(bucket_size, start_value, bucket_count)
    buckets =
      [lower_limits_buckets, upper_limits_buckets]
      |> Enum.zip_with(fn [x, y] -> "#{x}-#{y}" end)

    # If any values greater than range of last bucket,
    # put them into the last bucket
    # and change that bucket's name accordingly (e.g. "60-70" becomes "60+").
    case max_value < end_bucket do
      true ->
        Enum.zip(buckets, sums)

      false ->
        last_bucket = "#{start_value + bucket_count * bucket_size}+"
        last_sum =
          responses
          |> Helpers.frequencies(variable)
          |> Enum.filter(fn {key, _value} -> key >= end_bucket end)
          |> Enum.map(fn {_age, count} -> count end)
          |> Enum.sum

        sums = sums++[last_sum]
        buckets = buckets++[last_bucket]

        Enum.zip(buckets, sums)
    end
  end

Most Liked

ityonemo

ityonemo

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.

al2o3cr

al2o3cr

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!

sabiwara

sabiwara

Elixir Core Team

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.

Where Next?

Popular in Questions Top

9mm
I am constructing a JSON object (map) and I need to conditionally set a field. I’m trying to write proper elixir-way code… and I’m at a l...
New
chrisalley
ExUnit now has describe blocks which is a welcome addition coming from RSpec. In the docs, it states that nested hierarchies of describe ...
New
jononomo
I am trying to figure out how Mix knows whether the environment is test, dev, or prod – where is this set? Thanks.
New
minhajuddin
I have seen a lot of code which picks the first element from a list using Enum.at(0) instead of List.first. Is there a reason why people ...
New
Fl4m3Ph03n1x
About me? ( if you have nothing better to do than reading about some random guy in the internet :stuck_out_tongue: ) Hello all, this is ...
New
itssasanka
Hi all, Trying to get some more clarity over utc_datetime and naive_datetime for Ecto: The documentation above suggests that while ...
New
JDanielMartinez
Hi! May someone helps me, please! I have two apps into an umbrella project: the first one is Database, which manages queries, and the se...
New
fayddelight
I tried installing elixir 1.11.2 erlang 23.3.4 via asdf in my zsh shell. Enabled the versions locally and globally. When I list them ...
New
jason.o
In the code below, if the create action is not set to accept “extra_key” as an input, it errors out with a message shown above. Is there ...
New
komlanvi
Hi everyone, I was playing with phoenix liveView but I run into an issue. I have a form and want to validate each input text when the te...
New

Other popular topics Top

AstonJ
Posting this to see if we can make things easier for people to get into Neovim. If you use Neovim and have a favourite distro please let ...
New
minhajuddin
I have seen a lot of code which picks the first element from a list using Enum.at(0) instead of List.first. Is there a reason why people ...
New
chrismccord
This release brings a number of exciting features, including integration with the new Phoenix LiveDashboard and Phoenix LiveView. There h...
New
AngeloChecked
What learn first? Rust or Elixir Hi Elixir community! I’m here because i want learn a new language. I’m a junior developer and mainly i ...
New
gausby
I asked this very same question on twitter and got some interesting feedback, but I thought it would be a good question to ask here as we...
1207 39523 209
New
Qqwy
Original source of discussion: This topic on the Pragmatic Programmers’ Functional Web Development with Elixir, OTP, and Phoenix forum. ...
New
RisingFromAshes
I’ve read in another post that it may be possible with a router helper - but I couldn’t find an appropriate one, and tbh, I’m still just ...
New
bsollish-terakeet
Credo is smart enough to check for (something like) this: assert length(the_list) == 0 with this response: Checking if an enum is empt...
New
romenigld
I am trying to run a deploy with docker and I successfully runned with this command: docker build -t romenigld/blog-prod . but when I t...
New
sergio
Kind of like when jquery came out, it was super necessary. Existing drag and drop libraries have a bunch of baggage to support old browse...
New

We're in Beta

About us Mission Statement