Looks reasonable, though using lists can help avoid tricky off-by-one or off-the-end issues with elem.
As a demonstration of that principle, here are some refactors of the module:
defmodule RomanNumerals do
@doc """
Convert the number to a roman number.
"""
@table {{"M", 1000}, {"CM", 900}, {"D", 500}, {"CD", 400}, {"C", 100}, {"XC", 90}, {"L", 50}, {"XL", 40}, {"X", 10}, {"IX", 9}, {"V", 5}, {"IV", 4}, {"I", 1}}
@spec numeral(pos_integer) :: String.t()
def numeral(number) do
iterate(number, @table, 0, "")
end
defp iterate(num, table, index, out) do
if num == 0 do
out
else
{sym, ref} = elem(table, index)
cond do
num >= ref ->
iterate(num - ref, table, index, out <> sym)
true ->
iterate(num, table, index + 1, out)
end
end
end
end
This first refactor keeps the tuple-shaped table, but brings the symbols and the values together for readability and future maintainability. Having @table and @refs be different-sized tuples would be Not Good, so combining them together makes that bug impossible.
There’s another place for bugs to hide, though: when we write iterate(num, table, index+1, out), that will try to execute elem(table, index+1) and give:
** (ArgumentError) errors were found at the given arguments:
* 1st argument: out of range
:erlang.element(14, {{"M", 1000}, {"CM", 900}, {"D", 500}, {"CD", 400}, {"C", 100}, {"XC", 90}, {"L", 50}, {"XL", 40}, {"X", 10}, {"IX", 9}, {"V", 5}, {"IV", 4}, {"I", 1}})
iex:21: RomanNumerals.iterate/4
(this happened in an earlier version when I made a typo)
The root cause is that the recursive call to iterate assumes that a “next” element of table exists, without checking. You could add extra checks to ensure that index < tuple_size(table), but again it’s better to write code that literally can’t run off the end.
First, a very bad refactor that makes things slower. Enum.at is expensive compared to elem, since it needs to traverse the list. This also applies the early-exit cleanup that @benwilson512 suggested.
defmodule RomanNumerals do
@doc """
Convert the number to a roman number.
"""
@table [{"M", 1000}, {"CM", 900}, {"D", 500}, {"CD", 400}, {"C", 100}, {"XC", 90}, {"L", 50}, {"XL", 40}, {"X", 10}, {"IX", 9}, {"V", 5}, {"IV", 4}, {"I", 1}]
@spec numeral(pos_integer) :: String.t()
def numeral(number) do
iterate(number, @table, 0, "")
end
defp iterate(0, _, _, out), do: out
defp iterate(num, table, index, out) do
{sym, ref} = Enum.at(table, index)
cond do
num >= ref ->
iterate(num - ref, table, index, out <> sym)
true ->
iterate(num, table, index + 1, out)
end
end
end
This will fail in a slightly different way for an negative input (match error vs argument error) but it still “goes off the end” and crashes. Checking length(table) is expensive (traversing the list again!) so checking is even harder. Why am I telling you to use lists anyways?
Two things come together to make lists powerful here:
-
a call to
iteratewill only ever care aboutindexor higher intable -
There’s one place in a list that isn’t expensive to access: the first element (aka “the head”). It’s also easy to check for, since only
[]doesn’t have a head.
This refactor applies that principle: instead of keeping track of table and index separately, it uses hd and tl to interact with the first element and the rest. This is fast and requires no allocations.
defmodule RomanNumerals do
@doc """
Convert the number to a roman number.
"""
@table [{"M", 1000}, {"CM", 900}, {"D", 500}, {"CD", 400}, {"C", 100}, {"XC", 90}, {"L", 50}, {"XL", 40}, {"X", 10}, {"IX", 9}, {"V", 5}, {"IV", 4}, {"I", 1}]
@spec numeral(pos_integer) :: String.t()
def numeral(number) do
iterate(number, @table, "")
end
defp iterate(0, _, out), do: out
defp iterate(num, table, out) do
{sym, ref} = hd(table)
cond do
num >= ref ->
iterate(num - ref, table, out <> sym)
true ->
iterate(num, tl(table), out)
end
end
end
A small cleanup refactor: the pattern of “do something with hd and something else with tl” is so common that it’s usually not written explicitly. This uses pattern-matching to accomplish the same thing as the previous version
defmodule RomanNumerals do
@doc """
Convert the number to a roman number.
"""
@table [{"M", 1000}, {"CM", 900}, {"D", 500}, {"CD", 400}, {"C", 100}, {"XC", 90}, {"L", 50}, {"XL", 40}, {"X", 10}, {"IX", 9}, {"V", 5}, {"IV", 4}, {"I", 1}]
@spec numeral(pos_integer) :: String.t()
def numeral(number) do
iterate(number, @table, "")
end
defp iterate(0, _, out), do: out
defp iterate(num, [head | rest] = table, out) do
{sym, ref} = head
cond do
num >= ref ->
iterate(num - ref, table, out <> sym)
true ->
iterate(num, rest, out)
end
end
end
It’s also very common to absorb a binding like {sym, ref} = head into the function head:
defmodule RomanNumerals do
@doc """
Convert the number to a roman number.
"""
@table [{"M", 1000}, {"CM", 900}, {"D", 500}, {"CD", 400}, {"C", 100}, {"XC", 90}, {"L", 50}, {"XL", 40}, {"X", 10}, {"IX", 9}, {"V", 5}, {"IV", 4}, {"I", 1}]
@spec numeral(pos_integer) :: String.t()
def numeral(number) do
iterate(number, @table, "")
end
defp iterate(0, _, out), do: out
defp iterate(num, [{sym, ref} | rest] = table, out) do
cond do
num >= ref ->
iterate(num - ref, table, out <> sym)
true ->
iterate(num, rest, out)
end
end
end
This version crashes in yet a different way, so what can we do about it? The error message gives us a clue:
iex(26)> RomanNumerals.numeral(-1)
** (FunctionClauseError) no function clause matching in RomanNumerals.iterate/3
The following arguments were given to RomanNumerals.iterate/3:
# 1
-1
# 2
[]
# 3
""
iex:36: RomanNumerals.iterate/3
Think about what iterate being called with [] for table means: the conversion process has run out of symbols to try. That’s directly representable in code:
defmodule RomanNumerals do
@doc """
Convert the number to a roman number.
"""
@table [{"M", 1000}, {"CM", 900}, {"D", 500}, {"CD", 400}, {"C", 100}, {"XC", 90}, {"L", 50}, {"XL", 40}, {"X", 10}, {"IX", 9}, {"V", 5}, {"IV", 4}, {"I", 1}]
@spec numeral(pos_integer) :: String.t()
def numeral(number) do
iterate(number, @table, "")
end
defp iterate(0, _, out), do: out
defp iterate(_, [], _), do: raise("bad number")
defp iterate(num, [{sym, ref} | rest] = table, out) do
cond do
num >= ref ->
iterate(num - ref, table, out <> sym)
true ->
iterate(num, rest, out)
end
end
end
Another not-actually-relevant-here-but-worth-keeping-in-mind tip on performance: <> in a loop (or recursion) should be regarded with suspicion as it can result in lots of short-lived binaries. A common idiom avoids the repeated operations and does them at the end:
defmodule RomanNumerals do
@doc """
Convert the number to a roman number.
"""
@table [{"M", 1000}, {"CM", 900}, {"D", 500}, {"CD", 400}, {"C", 100}, {"XC", 90}, {"L", 50}, {"XL", 40}, {"X", 10}, {"IX", 9}, {"V", 5}, {"IV", 4}, {"I", 1}]
@spec numeral(pos_integer) :: String.t()
def numeral(number) do
iterate(number, @table, [])
end
defp iterate(0, _, out), do: out |> Enum.reverse() |> Enum.join()
defp iterate(_, [], _), do: raise("bad number")
defp iterate(num, [{sym, ref} | rest] = table, out) do
cond do
num >= ref ->
iterate(num - ref, table, [sym | out])
true ->
iterate(num, rest, out)
end
end
end
As a final cleanup, the cond with one non-default branch could be replaced with an if - or even a guard! The guard style makes the control structures disappear almost completely:
defmodule RomanNumerals do
@doc """
Convert the number to a roman number.
"""
@table [{"M", 1000}, {"CM", 900}, {"D", 500}, {"CD", 400}, {"C", 100}, {"XC", 90}, {"L", 50}, {"XL", 40}, {"X", 10}, {"IX", 9}, {"V", 5}, {"IV", 4}, {"I", 1}]
@spec numeral(pos_integer) :: String.t()
def numeral(number) do
iterate(number, @table, [])
end
defp iterate(0, _, out), do: out |> Enum.reverse() |> Enum.join()
defp iterate(_, [], _), do: raise("bad number")
defp iterate(num, [{sym, ref} | _] = table, out) when num >= ref do
iterate(num - ref, table, [sym | out])
end
defp iterate(num, [_ | rest], out) do
iterate(num, rest, out)
end
end
Apologies for the long post, but I wanted to avoid the usual “the steps between these two versions are OBVIOUS” hand-waving and justify each piece.
