5
\$\begingroup\$

As the title suggests, I'm a complete beginner in Haskell. I've just made my way through the "Input and Output" section of Learn You A Haskell For Greater Good and I thought it would be good to try my hand at a simple project like a "Guess The Word" game in Haskell.

The idea behind the game is to guess all the letters of a randomly selected word. If you guess all the letters, you win.

Without further ado, the complete program is below. I'm looking for feedback regarding program structure (particularly in separating I/O from pure functions), any inefficiencies I could look out for, any potential bugs, and any Haskell idioms and general best practices I could incorporate.

import Data.Char
import Data.Maybe
import System.Random

data Difficulty = Easy | Medium | Hard
    deriving (Read, Show, Eq)

createDifficulty :: String -> Maybe Difficulty
createDifficulty s = lookup s [
    ("e", Easy),
    ("m", Medium),
    ("h", Hard),
    ("", Easy)
    ]

isDifficulty :: String -> Bool
isDifficulty = isJust . createDifficulty

randomNumber :: Int -> Int -> IO (Int)
randomNumber begin end = do
    gen <- getStdGen
    return $ head $ randomRs (begin, end) gen

-- Please don't provide review about these tables. They're just meant to be demonstrative of the general idea I'm going for.
fetchWords :: Difficulty -> [String]
fetchWords Easy   = ["foo", "bar"]
fetchWords Medium = ["abc", "def"]
fetchWords Hard   = ["foobar", "whoami"]

generateSecret :: Difficulty -> IO (String)
generateSecret difficulty = do
    let table = fetchWords difficulty

    num <- randomNumber 0 (length table)

    return (table !! num)

replaceChar :: Int -> String -> Char -> String
replaceChar n s c = pre ++ [c] ++ tail post
    where (pre, post) = splitAt n s

compareSecret :: String -> String -> Int -> Char -> String
compareSecret secret guess pos c =
    if secret !! pos == c then
        replaceChar pos guess c
    else
        guess

isInt :: String -> Bool
isInt = all isDigit

isChar :: String -> Bool
isChar []     = False
isChar (c:cs) = isAlpha c && null cs

promptUntil :: (String -> Bool) -> String -> (String -> IO ()) -> IO (String)
promptUntil pred prompt failure = do
    putStr (prompt ++ "\n> ")
    input <- getLine
    if pred input then
        return input
    else do
        failure input
        promptUntil pred prompt failure

loop :: String -> String -> IO ()
loop secret guess = do

    putStrLn guess

    input1 <- promptUntil isInt "Enter position:" (\input -> putStrLn $ input ++ " is not a number.")
    input2 <- promptUntil isChar "Enter character:" (\input -> putStrLn $ input ++ " is not a character.")

    let pos = read input1 :: Int
    let c   = head input2

    let result = compareSecret secret guess pos c
    if result /= secret then
        loop secret result
    else
        return ()

main = do
    putStrLn "\n Welcome to Guess The Word!\n"

    difficultyString <- promptUntil isDifficulty "Enter difficulty level [e/m/h] or leave empty for easy difficulty: "
        (\input -> putStrLn $ input ++ " is not a valid difficulty.")

    let difficulty = fromMaybe (Easy) (createDifficulty difficultyString)

    putStrLn $ show difficulty ++ " difficulty selected."

    secret <- generateSecret difficulty

    loop secret $ replicate (length secret) '-'

    putStrLn secret
    putStrLn "You win!"
\$\endgroup\$
3
\$\begingroup\$

Overall the code looks really nice. I like the separation with a separate Difficulty type instead of doing the parsing directly in fetchWords.

Bugs

I have found three bugs:

Crash on invalid user input

If a user enters a number that is larger than the number of characters in the string, the program will crash. This is because you use the partial1 function (!!) without verifying its precondition that for xs !! i, i >= 0 and i < length xs. This could also be solved if you used a non-partial version of the function which returns a Maybe value so you can display a better error to the user and ask them to retry.

Random crash every 3:rd execution

There is also an off-by-one error in generateSecret, since randomRs generates an inclusive range and length xs is not a valid index for xs. This will cause the program to crash randomly on average once every length (fetchWords difficulty) + 1 times you play.

Only one random number

Speaking of randomness: the randomNumber function calls getStdGen, which doesn't update the RNG state, it just fetches it. This means that all random numbers will be the same. To make it update the state, you can either use randomRIO which will update the global RNG state automatically, or use newStdGen which will split the RNG state to allow you to generate random numbers from pure code. In this code you don't notice it though, since you only generate a single number.

The randomNumber function can be fixed by simplifying it to

randomNumber :: Int -> Int -> IO Int
randomNumber begin end = randomRIO (begin, end)

Idiomatic code

Partial functions

You use partial functions in a couple of places. Many of them can be avoided by using the principle "Parse, don't validate". In this case, all the "isX" functions can be replaced with getX functions that try to parse the data and Maybe returns a parsed value. The createDifficulty function is already such a function. This guarantees that the validation actually matches the code that uses it.

Here's an example of how it could be done. I have only included the relevant functions here. The index is validated, but there is no easy way to tell which list it is a valid index for unless you use some advanced trickery2.

import Text.Read (readMaybe)

newtype ValidIndex = ValidIndex Int

createDifficulty :: String -> Maybe Difficulty
createDifficulty s = lookup s [
    ("e", Easy),
    ("m", Medium),
    ("h", Hard),
    ("", Easy)
    ]

getInt :: String -> Maybe Int
getInt = readMaybe

isValidIndex :: [a] -> Int -> Bool
isValidIndex list idx = idx >= 0 && idx < length list

getValidIndex :: [a] -> String -> Maybe ValidIndex
getValidIndex list str = do
  idx <- getInt
  guard $ isValidIndex list idx
  return $ ValidIndex idx

getChar :: String -> Maybe Char
getChar [c] | isAlpha c = Just c
getChar []  = Nothing

promptUntil :: (String -> Maybe a) -> String -> String -> IO a
promptUntil parser prompt failureMessage = do
    putStr (prompt ++ "\n> ")
    input <- getLine
    case parser input of
      Just result -> return result
      Nothing -> do
        putStrLn $ input ++ failureMessage
        promptUntil parser prompt failure

loop :: String -> String -> IO ()
loop secret guess = do

    putStrLn guess

    pos <- promptUntil (getValidIndex secret) "Enter position:" " is not a valid index."
    c <- promptUntil getChar "Enter character:" " is not a character."

    let result = compareSecret secret guess pos c
    if result /= secret then
        loop secret result
    else
        return ()

main = do
    putStrLn "\n Welcome to Guess The Word!\n"

    difficulty <- promptUntil createDifficulty "Enter difficulty level [e/m/h] or leave empty for easy difficulty: "
        " is not a valid difficulty."

    putStrLn $ show difficulty ++ " difficulty selected."

    secret <- generateSecret difficulty

    loop secret $ replicate (length secret) '-'

    putStrLn secret
    putStrLn "You win!"

-- These functions are still partial, but we have checked their preconditions.
replaceChar :: ValidIndex -> String -> Char -> String
replaceChar (ValidIndex n) s c = pre ++ [c] ++ tail post
    where (pre, post) = splitAt n s

compareSecret :: String -> String -> ValidIndex -> Char -> String
compareSecret secret guess pos@(ValidIndex i) c =
    if secret !! i == c then
        replaceChar pos guess c
    else
        guess

Separating business logic from generic combinators

There are some cases where you can separate the code that is specific to your application from generic code that could be used anywhere.

For example, the generateSecret function can be split into a pickOne function:

pickOne :: [a] -> IO a
pickOne [] = error "pickOne: Empty list"
pickOne xs = do
    num <- randomRIO (0, length xs - 1)
    return $ xs !! num

generateSecret :: Difficulty -> IO (String)
generateSecret = pickOne . fetchWords

As you can see, the function is still partial, but since the word list is hard coded, we can probably assume that there is at least one word for each difficulty.

Explicit import lists

The recommended way of handling imports is to either use a qualified import like this:

import qualified Data.Char as Char

example = Char.isDigit '2'

or use an explicit import list like this

import Data.Char (isDigit)

example = isDigit '2'

This makes it easier for readers to see where functions come from.

Performance

Since all strings and lists are really short in this example, performance will not really be an issue. However, if the strings or lists were a lot longer, there are a few things to note. All functions that work with an index into a list are O(n) (instead of the O(1) they would be if we were working with a mutable array).

Haskell has support for mutable vectors, but then you lose the purity that is so nice about Haskell. If you want to keep purity, you can use a Data.Map, which has O(n*log n) complexity, but at the cost of a O(n*log n) memory cost. You could also use Data.Text, which still has O(n) complexity, but has a much more compact representation and is faster overall.

Anyways, as I said, this is not really relevant for this program since n (the length of the lists/strings) is really small and will probably never be large.


  1. A partial function is any function that can crash
  2. For example using Liquid Haskell or by using some fancy type-level trickery where the length of the list is in its type
\$\endgroup\$
1
  • \$\begingroup\$ Thank you so much for this in depth review; this was exactly what I was looking for! I'm going to read and study some of the material in the links you've provided -- especially the "parse don't validate" link. \$\endgroup\$ Dec 16 '19 at 14:43

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.