6
\$\begingroup\$

This little app converts 1-800 numbers (with letters) to actual numbers. I'm very frustrated by the insertDashes function as it's very brute force. I'm sure there's a far more elegant solution. I'd appreciate any feedback on anything else though.

module Dial
    ( enter,
    stripDashes
    ) where

import Data.Char (ord, toLower, isNumber, intToDigit, isPunctuation)
import System.IO
import Data.List


enter = do
    putStrLn "Enter a phone number"
    num <- getLine
    let converted = insertDashes $ convertLetters $ stripDashes num
    putStrLn $ "You may dial " ++ converted

stripDashes :: String -> String
stripDashes xs = [ x | x <- xs, not (elem x  "-") ]

insertDashes :: String -> String
insertDashes xs = take 1 xs ++ ['-'] ++ take 3 (drop 1 xs) ++ ['-'] ++ take 3 (drop 4 xs) ++ ['-'] ++ take 4 (drop 7 xs) 

getDigit :: Char -> Int
getDigit letter
    | (ord letter >= 97) && (ord letter <= 99) = 2
    | ord letter <= 102 = 3
    | ord letter <= 105 = 4
    | ord letter <= 108 = 5
    | ord letter <= 111 = 6
    | ord letter <= 115 = 7
    | ord letter <= 118 = 8
    | ord letter <= 122 = 9
    | otherwise = 0

convertLetters :: String -> String
convertLetters xs = map (\x -> if (isNumber x) then x else (intToDigit $ getDigit $ toLower x)) xs
\$\endgroup\$
5
\$\begingroup\$

Type signatures

Good job on providing all type signatures. Unfortunately, you forgot one: enter. Always include type signatures on top-level functions.

Remove unused imports

You use neither Data.List, nor System.IO.

Prefer character comparison instead of ord x > n

In getDigit, you compare characters by their ord values. That's not easy on the eye. It's a lot easier to read if you just compare the letter with characters:

getDigit :: Char -> Int
getDigit letter
    | letter >= 'a' && letter <= 'c' = 2
    | letter <= 'f' = 3
    | letter <= 'i' = 4
    | letter <= 'l' = 5
    | letter <= 'o' = 6
    | letter <= 's' = 7
    | letter <= 'v' = 8
    | letter <= 'z' = 9
    | otherwise = 0

There are other ways to write getDigit, but I they differ on personal preference only.

Use the standard library

stripDashes is filter (/= '-') in disguise:

stripDashes xs = [ x | x <- xs, not (elem x  "-")]
               = [ x | x <- xs, x /= '-']
               = filter (/= '-') xs

Therefore use filter instead of a list comprehension:

stripDashes :: String -> String
stripDashes = filter (/= '-')

By the way not (elem x y) is notElem x y.

Prefer local bindings instead of long lines

This is personal preference, but I think that

convertLetters :: String -> String
convertLetters xs = map toDigit xs
  where
    toDigit x 
     | isNumber x = x 
     | otherwise  = intToDigit $ getDigit $ toLower x

is easier to read.

Use hlint to get rid of redundant brackets

You have a lot of redundant brackets, which can make the code hard to read. hlint can report them to you. It also reports the not (elem x y) == notElem x y hint, by the way.

Prefer pure functions if possible

You have enter for a complete transformation. It would be great if there was a non-IO variant too, e.g.

convert :: String -> String
convert = insertDashes . convertLetters . stripDashes

That way you can export enter, conver and stripDashes.

Use splitAt instead of take/drop combinations

Finally, we have a look at insertDashes. First of all, let's write bindings:

insertDashes :: String -> String
insertDashes xs = group1 ++ ['-'] ++ group2 ++ ['-'] ++ group3 ++ ['-'] ++ group4 
   where
      group1 = take 1 xs
      group2 = take 3 (drop 1 xs)
      group3 = take 3 (drop 4 xs)
      group4 = take 4 (drop 7 xs)

That looks like a perfect job for splitAt:

-- do not include this function in your code, it's part of the Prelude
splitAt n xs = (take n xs, drop n xs)

However, if we were to rewrite insertDashes with splitAt, it wouldn't get any better immediately:

import Data.List (intercalate)

insertDashes :: String -> String
insertDashes xs = group1 ++ ['-'] ++ group2 ++ ['-'] ++ group3 ++ ['-'] ++ group4 
   where
      (group1,ys1) = splitAt 1 xs
      (group2,ys2) = splitAt 3 ys1
      (group3,ys3) = splitAt 3 ys2
      (group4,_  ) = splitAt 4 ys3

Ugh. Not really any better. So instead let's write another function:

splitAtNs :: [Int] -> [a] -> [[a]]
splitAtNs []     xs = [xs]
splitAtNs (n:ns) xs = as : splitAtNs ns bs
  where (as, bs) = splitAt n xs

Now insertDashes is

insertDashes :: String -> String
insertDashes xs = group1 ++ ['-'] ++ group2 ++ ['-'] ++ group3 ++ ['-'] ++ group4 
   where
      (group1:group2:group3:group4:_) = splitAtNs [1,3,3,4] xs

Which almost looks fine. There is one more improvement, though. The function intercalate from Data.List brings it down to a single line:

insertDashes :: String -> String
insertDashes = intercalate "-" . take 4 . splitAtNs [1,3,3,4]
\$\endgroup\$
  • \$\begingroup\$ This is FANTASTIC feedback. Thank you for taking to the time and for being so thorough! \$\endgroup\$ – Jim Wharton Mar 24 '18 at 15:01
  • \$\begingroup\$ @JimWharton thanks for YOUR feedback, highly appreciate it. By the way, do you know Hoogle or Hayoo yet? \$\endgroup\$ – Zeta Mar 24 '18 at 16:52
  • \$\begingroup\$ I do not. Most of my playing has been how do I <do this thing I've done in some other language for years>? That's not the best way to learn idiomatic Haskell, I'm certain. I've owned a copy of LYAH for about 4 years and haven't really applied it. This year I've been working with OCaml, F# and Haskell just for fun. These resources look great! \$\endgroup\$ – Jim Wharton Mar 24 '18 at 20:42

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.