10
\$\begingroup\$

I'm new to Elixir, and in order to learn the syntax, I'm doing a roman numeral kata which converts a decimal number into roman numeral. I would appreciate any feedback you have.

defmodule RomanNumeral do

  @decimal_roman_numerals [[5, 'V'], [4, 'IV'], [1, 'I']]

  def converts(number) do
    converts(number, [], @decimal_roman_numerals)
  end

  def converts(number, roman_value, _) when number < 1 do
    roman_value
  end

  def converts(number, roman_value, [[decimal, roman] | numerals]) when number >= decimal do
    converts(number - decimal, roman_value ++ roman, [[decimal, roman] | numerals])
  end

  def converts(number, roman_value, [_ | numerals]) do
    converts(number, roman_value, numerals)
  end
end
\$\endgroup\$
  • \$\begingroup\$ That is a very Prolog solution, which is good because Erlang is prolog inspired.] \$\endgroup\$ – Lyndon White Jan 18 '14 at 13:58
7
\$\begingroup\$

Your code looks very nice indeed, I only have some minor observations you code use to improve this code:

Naming
Use the imperative form of the verb (convert instead of converts)

Implementation Hiding
Use defp for methods which are not intended for external use. You might consider also renaming them to start with underscore _ to further differentiate them as internal.

Multiple assignment in method signature
To make you code more succinct, you can use multiple assignment - meaning that instead of:

def converts(number, roman_value, [[decimal, roman] | numerals]) when number >= decimal do
  converts(number - decimal, roman_value ++ roman, [[decimal, roman] | numerals])
end

you can write:

def converts(number, roman_value, [[decimal, roman] | _] = numerals) 
  when number >= decimal do
  converts(number - decimal, roman_value ++ roman, numerals)
end

to the same effect. This form assigns the first item in the list to [decimal, roman], as well as assigning the whole list (including the first element) to numerals, so you don't have to re-build it inside the method body.


Implementing the above suggestions, your code will look like this:

defmodule RomanNumeral do

  @decimal_roman_numerals [[5, 'V'], [4, 'IV'], [1, 'I']]

  def convert(number) do
    _convert(number, [], @decimal_roman_numerals)
  end

  defp _convert(number, roman_value, _) when number < 1 do
    roman_value
  end

  defp _convert(number, roman_value, [[decimal, roman] | _] = numerals) 
    when number >= decimal do
    _convert(number - decimal, roman_value ++ roman, numerals)
  end

  defp _convert(number, roman_value, [_ | numerals]) do
    _convert(number, roman_value, numerals)
  end
end
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.