Having read most of Learn You a Haskell as well as parts of Real World Haskell, I decided to embark on an easy project to see if I can apply the Haskell that I've learned.

{-# LANGUAGE OverloadedStrings #-}

import Data.Maybe
import Text.Regex.TDFA
import qualified Data.ByteString.Char8 as B

main = do
       dictionary <- B.readFile "words.txt"
       putStrLn "What is your phone number?"
       phoneNumber <- getLine
       B.putStrLn (outputMessage (getMatches dictionary (toPattern phoneNumber)))

outputMessage :: Maybe [B.ByteString] -> B.ByteString       
outputMessage Nothing   = "Some input was incorrect. Make sure no letters or numbers with 0 or 1 are being used."
outputMessage (Just []) = "Sorry. No matches were found."
outputMessage (Just x)  = "Matching words: \n" `B.append` ( B.unlines x)

getMatches _       Nothing        = Nothing
getMatches _       (Just "")      = Just []
getMatches words   (Just pattern) = if (words == B.empty) 
                                    then error "Was not able to read dictionary file."
                                    else Just (getAllTextMatches (words =~ (B.pack ("\\b" ++ pattern  ++ "\\b")) ))

toPattern = parseList . map toRegex . readNumber

parseList list 
    | Nothing `elem` list = Nothing
    | otherwise = Just (concat . catMaybes $ list)

toRegex :: (Data.String.IsString a1, Num a, Eq a) => a -> Maybe a1
toRegex 2 = Just "(a|b|c)"
toRegex 3 = Just "(d|e|f)"
toRegex 4 = Just "(g|h|i)"
toRegex 5 = Just "(j|k|l)"
toRegex 6 = Just "(m|n|o)"
toRegex 7 = Just "(p|q|r|s)"
toRegex 8 = Just "(t|u|v)"
toRegex 9 = Just "(w|x|y|z)"
toRegex _ = Nothing

readNumber :: [Char] -> [Integer]
readNumber = map $ read . (:"")

I'm interested in any suggestions for making my code more efficient/idiomatic.

  • 2
    \$\begingroup\$ a1? Ueh. We have b :P \$\endgroup\$ – Bartek Banachewicz Jun 23 '15 at 13:00
  • \$\begingroup\$ Sorry, I don't understand. \$\endgroup\$ – Joboman555 Jun 23 '15 at 13:27
  • 3
    \$\begingroup\$ @Joboman555 BartekBanachewicz is saying to write b instead of a1. \$\endgroup\$ – Veedrac Jun 23 '15 at 13:34

This is pretty nice code.

I have two main concerns. The first one is about handling errors.
You are mixing tree approaches: Maybe, throwing explicit error, throwing implicit error (as in readNumber).
It's better to stick to Maybe or Either — they are explicit and you can handle them in pure code.

The second one is about dividing your program in meaningful functions each having single responsibility. Such functions are simpler and you can more easily compose them.

For example, getMatches checks if dictionary is not empty, adds \b to pattern and matches pattern. It is better to split error handling from data processing (in this case by lifting dictionary check to main).

Another one is readNumber: it parses string into list of digits and implicitly checks for nondigits (throwing useless 'no parse' error). You can skip this as you check for invalid symbols in toRegex.

Some other remarks:

  • the type of toRegex is so general that it almost useless. I think you actually meant Integer -> Maybe String;
  • in parseList you first scanning list for Nothing and then catMaybes scanning it again to remove Nothings. It is possible to do this in single pass by using sequence.

Here is my take:

{-# LANGUAGE OverloadedStrings #-}

import Text.Regex.TDFA
import qualified Data.ByteString.Char8 as B
import Data.Monoid ((<>))
import Control.Monad (when)
import System.Exit (die)
-- `die` was introduced in the latest base, you can replace it with
-- die err = putStrLn err >> exitFailure

main = do
  dictionary <- B.readFile "words.txt"
  when (dictionary == "") $ die "Your dictionary is empty."

  putStrLn "What is your phone number?"
  phoneNumber <- getLine
  case toPattern phoneNumber of
    Nothing ->
      die "Some input was incorrect. Make sure no letters or numbers with 0 or 1 are being used."
    Just pattern -> case getAllTextMatches $ dictionary =~ pattern of
      []  -> B.putStrLn $ "Sorry. No matches were found."
      res -> B.putStrLn $ "Matching words: \n" <> B.unlines res

toPattern :: String -> Maybe B.ByteString
toPattern str = do
  parts <- sequence $ map toRegex str
  return $ "\\b" <> B.concat parts <> "\\b"

toRegex :: Char -> Maybe B.ByteString
toRegex = flip lookup
  [('2', "(a|b|c)")    ,('3', "(d|e|f)")
  ,('4', "(g|h|i)")    ,('5', "(j|k|l)")
  ,('6', "(m|n|o)")    ,('7', "(p|q|r|s)")
  ,('8', "(t|u|v)")    ,('9', "(w|x|y|z)")

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.