# Converting to Roman Numerals exercise; need help understanding code

I have a hard time following the code at `defp numeral` function. Is this well written Elixir code or is it better to use pipes?

``````defmodule Roman do
@numerals [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]

@doc """
Convert the number to a roman number.
"""
@spec numerals(pos_integer) :: String.t()
def numerals(number) do
numeral([], number, @numerals)
end

defp numeral(curr_nums, number, [{letter, digit}|_t] = num_list) when number >= digit do
numeral([letter | curr_nums], number - digit, num_list)
end

defp numeral(curr_nums, number, [_h|t]) do
numeral(curr_nums, number, t)
end

defp numeral(numerals, _n, []) do
numerals
|> Enum.reverse
|> Enum.map_join("", &to_string/1)
end
end
``````

What are some suggestions for re-writing this in a better way? It’s my old code but I no longer understand what is happening.

1 Like

I think its fine except the variable naming makes it a bit hard to see what’s happening. And the odd comment helps. Personally I think a recursive function like this is a good fit for the problem. Perhaps like this:

``````defmodule Roman do
@roman_to_decimal_mappings [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]

@doc """
Convert the number to a roman number.
"""
@spec numerals(pos_integer) :: String.t()
def numerals(number) do
numeral([], number, @roman_to_decimal_mappings)
end

# While the number is >= the decimal value at the head of the mappings,
# accumulate the Roman numeral and decrement the number
defp numeral(roman_acc, number, [{roman_numeral, decimal_value}| _t] = roman_to_decimal_mappings)
when number >= decimal_value do
numeral([roman_numeral | roman_acc], number - decimal_value, roman_to_decimal_mappings)
end

# When the number is less than the decimal value at the head of the
# mapping list, move to the next mapping pair
defp numeral(roman_acc, number, [_h | remaining_mappings]) do
numeral(roman_acc, number, remaining_mappings)
end

# And when the mapping list is exhausted, reverse the accumulated
# Roman numerals and join then
defp numeral(roman_acc, _n, [] = _remaining_mappings) do
roman_acc
|> Enum.reverse
|> Enum.map_join("", &to_string/1)
end
end
``````
2 Likes

I’m not certain how readable this is for others, but it’s how I worked through the problem early in my Elixir adventures, and I can still explain the approach 6 years later. https://github.com/gvaughn/elixir_kata/blob/master/roman/roman_numerals.exs

I iterate over the mappings and use `Enum.reduce` with a 2-tuple to keep track of the remainder of the number (after dividing out each Roman value) plus the accumulated Roman numeral string.

4 Likes