HiveBrain v1.2.0
Get Started
← Back to all entries
snippetMinor

Convert Roman Numeral to Arabic

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
convertarabicnumeralroman

Problem

I've been learning Haskell recently and I decided I needed to work on a (somewhat) realistic problem.

What I'm most interested in getting feedback on is how well I've used the tools in Haskell to accomplish the goal of converting Roman numerals to Arabic values. I suspect that my approach is more verbose than it needs to be, so thoughts on how I could remove unneeded code would be quite welcome.

Lastly, thoughts on how to address Haskell bad practices or similar errors would be appreciated.

```
-- file: roman.hs
import Data.Char

-- Application startup.
main :: IO ()
main = do
putStrLn "Enter a Roman numeral. The numerical value will be returned."
putStrLn "For example MMI -> 2001."
putStrLn "Enter 'finis' to exit."
process
putStrLn "vale! :-)"

-- Main loop.
process :: IO ()
process = do
putStrLn "Numeral: "
line [Numeral]
readNumerals (x:xs) = readNumeral (toUpper(x)) : [] ++ readNumerals xs -- First cons the Char to an empty list, then recur.
readNumerals "" = []

-- Parse each char into the corresponding numeral.
readNumeral :: Char -> Numeral
readNumeral 'I' = Numeral 1 'I' ['V', 'X']
readNumeral 'V' = Numeral 5 'V' []
readNumeral 'X' = Numeral 10 'X' ['L', 'C']
readNumeral 'L' = Numeral 50 'L' []
readNumeral 'C' = Numeral 100 'C' ['D', 'M']
readNumeral 'D' = Numeral 500 'D' []
readNumeral 'M' = Numeral 1000 'M' []
readNumeral _ = errorNum

-- Turn a list of Roman numerals into their corresponding Arabic number.
getValue :: [Numeral] -> Int
getValue [] = 0
getValue (x:xs) =
if subRuler (subRule x) xs
then negate (numeralValue x) + getValue xs
else numeralValue x + getValue xs
-- if x has a non-empty subRule and the next x is in the list, then flip the sign for x

-- Take the subRule from x and xs, if x of xs is in subRule, return true, otherwise false.
-- What this really means is see if the next element in the list of Numerals is one of the elements in subRule.
s

Solution

Parens

When you call a function you do not need to place the argument in parens:

Instead of:  readNumerals(line)
       Use:  readNumerals line


Error Handling

In Haskell we use types like Either or Maybe to indicate errors. Instead of:

let nums = readNumerals(line)
if errorNum `elem` mums
  then ...some error...
  else ...


You should define readNumerals to have this type:

readNumerals :: String -> Maybe [Numeral]


and write:

case readNumerals line of
  Nothing -> ... some error ...
  Just ns -> ... parse was valid, numerals are in `ns` ...


data Numeral

The data structure Numeral has many fields which are redundant. For instance, if n is a Numeral and numeralValue n is 1, then it will always be the case that numeralSymbol n will be I and subRules n will be ['V','X']. So there's no point storing these in the record - you can just implement these as functions:

subRules :: Numeral -> [Char]
subRules n = case numeralValue n of
                1  -> ['V','X']
                5  -> []
                10 -> ['L', 'C']
                ...


This allows you to eliminate the subRules field from the record.

Make Numeral an ADT

In fact, Numeral doesn't even need any fields. Here's a simpler way to implement Numeral:

data Numeral = I | V | X | L | C deriving (Show, Eq)

numeralValue :: Numeral -> Int
numeralValue I = 1
numeralValue V = 5
...

subRule :: Numeral -> [Numeral]
subRule I = [V, X]
subRule V = []
...


The nice thing is that the code for getValue doesn't change at all. The function subRuler does change a little (and also becomes simpler):

subRuler :: [Numeral] -> [Numeral] -> Bool
subRuler [] _ = False
subRuler _ [] = False
subRules as bs = (head bs) `elem` as


Avoid using head

head is a partial function - i.e. it could throw an error. Here's how to write subRuler without using head:

subRules [] _ = False
subRuler _ [] = False
subRuler as (b:_) = b `elem` as


We've replaced head with pattern matching, and it does the same thing. tail is also a partial function which you should replace with pattern matching.

Code Snippets

Instead of:  readNumerals(line)
       Use:  readNumerals line
let nums = readNumerals(line)
if errorNum `elem` mums
  then ...some error...
  else ...
readNumerals :: String -> Maybe [Numeral]
case readNumerals line of
  Nothing -> ... some error ...
  Just ns -> ... parse was valid, numerals are in `ns` ...
subRules :: Numeral -> [Char]
subRules n = case numeralValue n of
                1  -> ['V','X']
                5  -> []
                10 -> ['L', 'C']
                ...

Context

StackExchange Code Review Q#127429, answer score: 2

Revisions (0)

No revisions yet.