3
\$\begingroup\$

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 <- getLine
    if line == "finis"
        then return ()
        else do
            let nums = readNumerals(line)
                in if errorNum `elem` nums
                    then putStrLn (line ++ "is not a valid Roman numeral.")
                    else putStrLn ("   The Roman numeral " ++ line ++ " is equal to " ++ show(getValue(nums)))
            process

data Numeral = Numeral {
    numeralValue :: Int,
    numeralSymbol :: Char,
    subRule :: [Char]}
    deriving (Show, Eq)

-- This is the Numeral error type. used to check if the input was valid.
errorNum = Numeral 0 'E' []

-- Turn a string into a list of numerals.
readNumerals :: String -> [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.
subRuler :: [Char] -> [Numeral] -> Bool
subRuler [] _ = False
subRuler _ [] = False
subRuler a b = numeralSymbol (head b) `elem` a

{-
Subtractive Rules, from Wikipedia
I placed before V or X indicates one less, so four is IV (one less than five) and nine is IX (one less than ten)
X placed before L or C indicates ten less, so forty is XL (ten less than fifty) and ninety is XC (ten less than a hundred)
C placed before D or M indicates a hundred less, so four hundred is CD (a hundred less than five hundred) and nine hundred is CM (a hundred less than a thousand)
-}
\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thank you for the comments and for providing me so many things to think about. \$\endgroup\$ – Seth May 13 '16 at 13:24
-2
\$\begingroup\$

There is a monoid form that can handle arbitrary length Roman numerals. Hackish Ruby code.

It is actually more abstract because it handles not just "IV"=4 and "IX"=9 but also strings like "IM"=999. That allows you to get rid of the subRule :: [Char].

\$\endgroup\$
  • \$\begingroup\$ Hi. Welcome to Code Review! Unfortunately, this isn't really what we mean by an answer. We prefer answers to review the code in the question. This usually involves quoting some code from the question, posting an alternative version of that code (in the same language), and then explaining why the new code is better than the original. Our goal is less to solve programming problems and more to help people become better coders. While I haven't downvoted your answer, I suspect that those who have did so for this reason. \$\endgroup\$ – mdfst13 May 4 '16 at 22:38
  • \$\begingroup\$ The link is to my working Ruby code that uses a monoid pattern. Folds over monoids are usually preferred in Haskell over recursion. \$\endgroup\$ – Chad Brewbaker May 5 '16 at 5:46
  • \$\begingroup\$ Thanks for the review. I'm not going to accept your answer, as I feel it doesn't address many of the questions I asked. However, you've piqued my interest. Can you explain why folds over monoids are preferred? \$\endgroup\$ – Seth May 10 '16 at 14:13
  • \$\begingroup\$ I'll try to find a block of time rewrite it into Haskell. Monoids (associative binary operations with a special "empty" element) have all these things available to them, hackage.haskell.org/package/base-4.8.2.0/docs/Data-Monoid.html Also see Avi's talk on this at Strangeloop 2013, infoq.com/presentations/abstract-algebra-analytics \$\endgroup\$ – Chad Brewbaker May 10 '16 at 20:06

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.