6
\$\begingroup\$

This converts a "numerical number" to a text representation, for example:

convert 1234 --> "One Thousand, Two Hundred Thirty Four"

I'd like general feedback on how it could be make more readable or idiomatic. There are a few specific points I'd like improved as well. The word lookup functions are a little messy. Each uses a different method of looking up the name depending on how the input relates to the desired name. If they could be generalized, it would be much neater. It's also not that efficient. It requires a few reversals, all of which are necessary with my current technique; as far as I can tell.

import Data.Char
import Data.List

-- Global names
places  = ["","Thousand","Million","Billion","Trillion","Quadrillion","Quintillion"]
ones    = ["One","Two","Three","Four","Five","Six","Seven","Eight","Nine"]
teens   = ["Ten","Eleven","Twelve","Thirteen","Fourteen","Fifteen","Sixteen","Seventeen","Eighteen","Nineteen"]
tens    = ["A 1 made it to getTens","Hundred","Twenty","Thirty","Forty","Fifty","Sixty","Seventy","Eighty","Ninety"]

-- Word lookup functions
getPlaceName :: Int -> String
getPlaceName place = places !! place

getOnes :: Char -> String
getOnes '0' = ""
getOnes n = let index = (digitToInt n) - 1 in
    ones !! index

getTeens :: String -> String
getTeens n = let r = read n :: Int in
    head $ [x | (i,x) <- (zip [10..] teens), i == r]

getTens :: Char -> String
getTens '0' = ""
getTens n   = let index = (digitToInt n) in
    tens !! index

--Takes a string rep. of a number, reverses it, and groups it into 3-groups
-- 12,000 -> ["000", "21"]
cutIntoGroups :: String -> [String]
cutIntoGroups = reverse . cut [] . reverse
    where
        cut acc []      = acc
        cut acc rest    = let (next,rest') = splitAt 3 rest in
            cut (next : acc) rest'

-- Applies the "rules" to each sub-chunk 
applyLocalRules :: String -> String
applyLocalRules revChunk = hund ++ tensNOnes
    where
        (ones,tens,hunds) = toTriple revChunk '0'
        tensNOnes = if tens == '1' then (getTeens [tens,ones])
                        else (getTens tens) ++ (getOnes ones)
        hund = if hunds == '0' then ""
                    else (getOnes hunds) ++ "Hundred"

-- Succeedes each formatted chunk with the place name                   
applyRules :: [String] -> [String]
applyRules = reverse . map (\(i,g) -> let pName = getPlaceName i in
    applyLocalRules g ++ pName) . zip [0..]

toTriple :: [a] -> a -> (a,a,a)
toTriple [] def = (def,def,def)
toTriple xs def = case xs of
    [x,y,z] -> (x,y,z)
    [x,y]   -> (x,y,def)
    [x]     -> (x,def,def)
    _       -> error "List greater then 3"

addCommasAndConcat :: [String] -> String
addCommasAndConcat = concat . map (\chunk -> chunk ++ ",")

-- Adds spaces before words
addSpaces :: String -> String
addSpaces [] = []
addSpaces left = (h : pre) ++ " " ++ addSpaces post
    where
        (h:t)       = left
        (pre,post)  = break isUpper t

-- Loosely stolen from SO       
trim :: String -> String
trim = let t = reverse . dropWhile (not . isAlpha) in
    t . t

-- Main function    
convert :: Int -> String
convert = trim . addSpaces . addCommasAndConcat
    . applyRules . cutIntoGroups . show

It should be noted that this is very unsafe in its current form, so I'm only looking for comments regarding the algorithm itself (it crashes ungracefully on negative numbers, non-numbers, and too-large of numbers). I think I'm going to use this module to test out the Either Monad later to fix its safety.

\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

I added the following test code to investigate. It does an exhaustive check from 1 to 999 and then a few more cases.

import Data.Monoid
import Control.Monad
import Test.Hspec

specTest n w = it ("converts " <> show n) $ convert n `shouldBe` w

oneToNine = words "One Two Three Four Five Six Seven Eight Nine"
zeroToNineEndings = "" : map ("-"<>) oneToNine
oneToNinetyNine = oneToNine
    <> words "Ten Eleven Twelve Thirteen Fourteen Fifteen Sixteen Seventeen Eighteen Nineteen"
    <> [decade <> ending | decade <- words "Twenty Thirty Forty Fifty Sixty Seventy Eighty Ninety", ending <- zeroToNineEndings]

main = hspec $ do
  zipWithM_ specTest [1..]
    $ oneToNinetyNine
    <> mconcat
      [[d <> " Hundred"] <> map ((d <> " Hundred ") <>) oneToNinetyNine
      | d <- oneToNine
      ]

  specTest 1000 "One Thousand and "
  specTest 1001 "One Thousand and One"
  specTest 1234 "One Thousand and Two Hundred Thirty-Four"
  specTest 1234567 "One Million, Two Hundred Thirty-Four Thousand and Five Hundred Sixty-Seven"

Note that 1000 converts to 'One Thousand and ' which is probably wrong. Also, your original question claims 1234 converts to 'One Thousand, Two Hundred Thirty Four' but in fact it comes out as 'One Thousand and Two Hundred Thirty-Four'.

I am not sure your rules for adding 'and' and commas in are correct, however I'm from the UK where we typically put an 'and' between the hundreds and tens (e.g. 'two hundred and thirty-four') and I'm aware that this isn't how things are done in the US. I'd recommend adding some more test cases as I've shown above to make sure you're getting things as you expect.

The code seems idiomatically ok, although the mixture of CamelCase and use_of_underscores is a little jarring. hlint offers some good advice on the use of idiom and reports this, a few unnecessary parentheses and $s and identifies that concat . intersperse can be written as intercalate. Nothing major.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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