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
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
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!
sabiwara
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.
Popular in Questions
Other popular topics
Categories:
Sub Categories:
Forums
Popular Tags
- #ecto
- #liveview
- #troubleshooting
- #learning-elixir
- #deployment
- #library
- #erlang
- #testing
- #genserver
- #mix
- #absinthe
- #remote-other
- #otp
- #plug
- #how-to-question
- #macros
- #postgres
- #channels
- #elixirconf
- #exunit
- #discussion
- #code-sync
- #javascript
- #podcasts
- #onsite
- #dialyzer
- #docker
- #authentication
- #umbrella
- #full-time-contract
- #podcasts-by-brainlid
- #ecto-query
- #elixir-ls
- #phoenix_html
- #iex
- #blog-post
- #graphql
- #genstage
- #ai
- #websockets
- #supervisor
- #advent-of-code
- #elixirconf-us
- #distillery
- #processes
- #forms
- #api
- #metaprogramming
- #security
- #performance









