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  ## 1 Answer # 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


# 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]

• This is FANTASTIC feedback. Thank you for taking to the time and for being so thorough! – Jim Wharton Mar 24 '18 at 15:01
• @JimWharton thanks for YOUR feedback, highly appreciate it. By the way, do you know Hoogle or Hayoo yet? – Zeta Mar 24 '18 at 16:52
• 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! – Jim Wharton Mar 24 '18 at 20:42