Berlin clock kata in Elixir

Question

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
        ""
    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(String.duplicate("Y", number_of_y), 4, ?O)
    end

    def get_seconds(s) do
        cond do
            s |> even? -> "Y"
            s |> odd?  -> "O"
        end
    end
end

Answer 1

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.

Answer 2

First: I have to admit, I do not know a single line Elixir.

But on the other hand, I think I get, what the code does. I can not give you advice, how to implement the improvements, but I hope to help you anyway.

In the description you have the following definition:

single minutes:

When it is 1 minute past the hour, YOOO is returned.

When it is 2 minutes past the hour, YYOO is returned.

When it is 5 minutes past the hour, OOOO is returned.

single hours:

When it is 1 hour past, ROOO is returned.

When it is 2 hours past, RROO is returned.

When it is 5 hours past, OOOO is returned.

What both have in common is the pattern 1,2,5 and the result *000, **00, 0000.

That should result in one function, which takes the following parameters: number,Symbol and produces the according output.

Your code has:

def get_single_minutes(m) do
    number_of_y = rem(m, 5)
    String.ljust(String.duplicate("Y", number_of_y), 4, ?O)
end

and

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

which should be refactored to one function.

If you take a look at

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

and the above function(s) you see another pattern in common:

number_of_whatsoever = X

Where X is the result of a function: either div(h,5) or rem(h,5). So the next step is refactoring number,Symbol => output to number,Symbol, fn => output. You get 3 lines instead of 9.

The only tricky part is the 5 minutes row

You could go the road of abstraction one step further and make one single function for each line:

f(emptyline, functionToFillLines)=>output

1) For seconds

f('O', fillSeconds)=>output

2) For single minutes, single hours, five hours

f('OOOO', fillFunction)=>output

3) For 5 minutes

f('OOOOOOOOOOO', fill5minutes)=>output

Where the fillX-Functions were closures, taking one argument as input.