snippetMinor
Convert Roman Numeral to Arabic
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
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:
Error Handling
In Haskell we use types like
You should define
and write:
data Numeral
The data structure
This allows you to eliminate the
Make Numeral an ADT
In fact,
The nice thing is that the code for
Avoid using
We've replaced
When you call a function you do not need to place the argument in parens:
Instead of: readNumerals(line)
Use: readNumerals lineError 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` asAvoid using
headhead 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` asWe'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 linelet 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.