4
votes

I've recently started learning Haskell and currently trying to writing basic Haskell functions.

I've written a function called intToRoman which should convert Integer number to Roman number. It divides the number in list of integers ( 1400 ->[1,4,0,0]) and converts every number into the Roman number taking into the account length of list to determine whenever is a thousand or hundred.

However, it doesn't stop and checks zeros as well. For instance, number 1400 will return:

 MCD** Exception: Map.!: given key is not an element in the map
 CallStack (from HasCallStack)

Here is the code itself:

mapInttoRoman:: M.Map Int String
mapInttoRoman = M.fromList
          [(1,"I"),(4,"IV"),(5,"V"),(9,"IX"),(10,"X"),(40,"XL")
          ,(50,"L"),(100,"C"),(400,"CD"),(900,"CM"),(500,"D"),(1000,"M")]

listOfInt :: Int -> [Int]
listOfInt 0 = [ ]
listOfInt c = listOfInt(c`div`10) ++ [c`mod`10]

duplicate :: String -> Int -> String
duplicate string n = concat $ replicate n string

intToRoman :: Int -> String
intToRoman 0 = ""
intToRoman c = createInt x (len-1)
 where x = listOfInt c
       len = length x
       createInt y xz = findRoman (head y) xz ++ createInt(drop 1 y)(xz-1)
        where findRoman b l
               | l < 1      = mapInttoRoman M.! b
               | b == 0     = " "
               | l == 3     = duplicate (mapInttoRoman M.! (10^l)) b
               | b == 1     = mapInttoRoman M.! (10^l)
               | otherwise  = mapInttoRoman M.! (b*10^l)
1
Your | b == 0 = " " case fails to catch this because the | l < 1 = .. case is checked first. You can switch their order. (It'll then fail because you try to get the head of an empty list, so you'll have to add a base case for createInt [] _ = "") - that other guy
I understand that you may be new to Haskell, but please consider reading the style guide. This is not good code. - AJF
@AJFarmar I don't think that is the right thing at all to worry about when you're learning a new language with lots of new concepts and writing programs just for your own education. A style guide matters for programs that need to be maintained. - amalloy

1 Answers

0
votes

I think the Algorithm you use is not correct, and you made it more complicated than it needs to be.

Also the list was not complete, I added 90 and 900 cases.

Have a look at this code .. I think it's very simple and you will get it easily.

import qualified Data.Map as M

    mapInttoRoman:: M.Map Int String
    mapInttoRoman = M.fromList
              [(1,"I"),(4,"IV"),(5,"V"),(9,"IX"),(10,"X"),(40,"XL")
              ,(50,"L"),(90,"XC"),(100,"C"),(400,"CD"),(500,"D"),(900,"CM"),(1000,"M")]

    intToRoman :: Int -> String
    intToRoman 0 = ""
    intToRoman n | M.member n mapInttoRoman = mapInttoRoman M.! n
                 | otherwise = val ++ intToRoman (n-key) 
                 where
                    keys = reverse (M.keys mapInttoRoman)
                    key  = maxValidKey n keys
                    val  = mapInttoRoman M.! key
                    maxValidKey n (k:ks) 
                      | n >= k = k
                      | otherwise = maxValidKey n ks 

Tests :

enter image description here