Return to Revisions

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 - you 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.