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: Enum.reverse(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:
[h, m, s] = _parse_time(time, [])
It is not worse or better than your solution, but it uses a different Elixir idiom.