Berlin clock kata in Elixir

I'm starting with Elixir and as exercise I wrote the Berlin Clock kata:

• The clock is made up of 5 rows.

• On the very top of the clock is a lamp that blinks to show the seconds. It turns on for one second, then off for one second, and so on.

• The next 2 rows represent the hours. The upper most of these rows represents 5 hour blocks and is made up of 4 red lamps. The second row represents 1 hour blocks and is also made up of 4 red lamps.

• The final two rows represent the minutes. The upper most row repesents 5 minute blocks and is made up of 11 lamps- every 3rd lamp red but the rest yellow. The second row represents 1 minute blocks and is made up of 4 yellow lamps.

Here's a picture:

The full code is available here.

```
import Integer, only: :macros

defmodule BerlinClock do
def parse (time) do

[h, m, s] = String.split(time, ":")
|> Enum.map fn n -> Integer.parse(n)
|> elem(0) end

seconds = get_seconds(s)
single_minutes = get_single_minutes(m)
fives_minutes = get_fives_minutes(m)
single_hours = get_single_hours(h)
fives_hours = get_fives_hours(h)

[seconds, fives_hours, single_hours, fives_minutes, single_minutes]
end

def get_fives_hours(h) do
number_of_r = div(h, 5)
String.ljust(String.duplicate("R", number_of_r), 4, ?O)
end

def get_single_hours(h) do
number_of_r = rem(h, 5)
String.ljust(String.duplicate("R", number_of_r), 4, ?O)
end

def get_fives_minutes(m) do
number_of_y = div(m, 5)
String.ljust(create_fives_minutes(number_of_y), 11, ?O)
end

def create_fives_minutes(1) do
"Y"
end

def create_fives_minutes(n) when n 1 do
if rem(n,3) == 0 do
light = "R"
else
light = "Y"
end
[create_fives_minutes(n-1), light ] |> Enum.join
end

def get_single_minutes(m) do
number_of_y = rem(m, 5)
String.ljust

Tail Recursion

Erlang (and hence Elixir) pride in the tail recursion idioms they use. You've got one recursion in your code but it is not a tail recursion!

This means that the recursive part is not the last thing in the function, and the tail recursion optimization cannot be used. Although your code's recursion is limited to up to 11 hits, it still misses the point of the Elixir idiom.

A more idiomatic pattern is using an accumulator, and recursing on that:

def create_fives_minutes(m) when m >= 0, do: _create_fives_minutes(m, "")

def _create_fives_minutes(0, acc), do: acc

def _create_fives_minutes(m, acc) when rem(m,3) == 0 do
  _create_fives_minutes(m-1, "R" <> acc)
end

def _create_fives_minutes(m, acc), do: _create_fives_minutes(m-1, "Y" <> acc)

if code smell

In Elixir if and cond are seldom used, and are even considered a code smell - if you can solve the problem using pattern matching - it is considered more idiomatic. Consider get_seconds - the following is more idiomatic in Elixir than using cond:

def get_seconds(s) when even?(s), do: "Y"
def get_seconds(_), do: "O"

Also see above how I've avoided the if in the create_fives_minutes above using a guard condition (when rem(m,3))

Code Duplication

You've got several methods, which do essentially the same thing. Making a method which takes the duplicate code will remove the need for them entirely - something like:

def build_field(pattern, repeat \\ 1, field_length) do
    String.ljust(String.duplicate(pattern, repeat), field_length, ?O)
end

Now - your main code can look something like this:

seconds = get_seconds(s)
    single_minutes = build_field("Y", rem(m, 5), 4)
    fives_minutes = build_field(create_fives_minutes(div(m, 5)), 11)
    single_hours = build_field("R", rem(h, 5), 4)
    fives_hours = build_field("R", div(h, 5), 4)

and all the helper methods are no longer needed.

Indentation and readability

When parsing the time string you've got a piping block which looks like this:

[h, m, s] = String.split(time, ":") 
        |> Enum.map fn n -> Integer.parse(n) 
        |> elem(0) end

This is a very misleading indentation, since you indent elem(0) at the same level as the Enum.map.

A more readable way to put it should be:

[h, m, s] = String.split(time, ":") 
        |> Enum.map fn n -> 
            Integer.parse(n) 
            |> elem(0)
        end

Showing that |> elem(0) is inside the Enum.map function, and not after it.

Playing with the idioms

You can parse the time string using the recursive method idioms, which would look like this:

def _parse_time("", acc), do: acc

def _parse_time(":" <> s, acc), do: _parse_time(s, acc)

def _parse_time(s, acc) do
    {num, s} = Integer.parse(s)
    _parse_time(s, [num | acc])
end

and used like this:

[s, m, h] = _parse_time(time, [])

It is not worse or better than your solution, but it uses a different Elixir idiom.

StackExchange Code Review Q#58690, answer score: 6