3
\$\begingroup\$

I've been learning a lot about functional programming lately and I've recently started learning Haskell to which I'm very new.

I've written a simple number guessing game and I would like to have feedback on what is good or bad about my code and how it could be improved.

#!/usr/bin/env runhaskell

import Text.Printf (printf)
import System.Random (randomRIO)

smallest = 1 :: Integer
biggest = 100 :: Integer

main :: IO ()
main = do
  secret <- randomRIO (smallest, biggest) :: IO Integer
  printf "Find the number between %d and %d.\n" smallest biggest
  loop secret 1 where
    loop :: Integer -> Integer -> IO ()
    loop secret tries = do
      guess <- readLn :: IO Integer
      case compare guess secret of
        LT -> do
          putStrLn "Too small!"
          loop secret $ succ tries
        GT -> do
          putStrLn "Too big!"
          loop secret $ succ tries
        EQ -> do
          printf "You found the number in %d tries!\n" tries
\$\endgroup\$
1
  • 1
    \$\begingroup\$ I think it would be good practice to handle non numeric inputs instead of throwing an exception. Maybe even add an input to quit the program. \$\endgroup\$ – bdecaf Jul 14 '16 at 12:00
2
\$\begingroup\$

Changing the where to a let allows you to skip passing the secret. Passing guesses in and messages out purifies the control flow part of the program. zip [1..] makes clear: The number of tries during each step does not depend on the control flow. interact, lines and foldr replace the recursion. (Recursion is too powerful for brains to handle in general, so we should avoid it when we have a simpler tool.) Your mileage may vary on any of these suggestions.

secret <- randomRIO (smallest, biggest) :: IO Integer
let foo :: (Integer, Integer) -> String -> String
    foo (tries, guess) = case compare guess secret of
      LT -> (++) "Too small!\n"
      GT -> (++) "Too big!\n"
      EQ -> const $ printf "You found the number in %d tries!\n" tries
printf "Find the number between %d and %d.\n" smallest biggest
interact $ foldr foo undefined . zip [1..] . map read . lines
\$\endgroup\$
1
\$\begingroup\$

Overall, good work. It's a solid first number guessing game.

The good

You're using type annotations. That's great. The type inference actually needs only two annotations (one of secret, smallest or biggest, and tries).

The bad

However, your program fails if the user does not enter a number:

Find the number between 1 and 10.
Hello
*** Exception: user error (Prelude.readIO: no parse)

That's not very user friendly. More on that in the next section.

The ugly

While I don't concur with @Gurkenglas that recursion is too powerful for brains, there are some weird things going on with loop:

  • you have to thread secret throughout all recursive calls
  • you use succ tries instead of tries + 1

Good code is written for humans and machines, but the latter are second class citizens. So let's try to get rid of some noise:

main :: IO ()
main = do
  secret <- randomRIO (smallest, biggest) :: IO Integer
  printf "Find the number between %d and %d.\n" smallest biggest
  let loop tries :: Int -> IO ()
      loop tries = do
        guess <- askNumberOrQuit
        case compare guess secret of
          LT -> do
            putStrLn "Too small!"
            loop (tries + 1)
          GT -> do
            putStrLn "Too big!"
            loop (tries + 1)
          EQ -> do
            printf "You found the number in %d tries!\n" tries
  loop

The function askNumberOrQuit is left as an exercise, but for a first implementation you can think of it as

askNumberOrQuit :: IO Integer
askNumberOrQuit = readLn

There are more elegant ways to handle this, e.g. prompt the user again if they didn't write a number, or quit if the user writes q[uit].

Either way, loop is now a lot larger than the original main. And since we have to indent it with let, there's a lot of whitespace on the left hand side, usually a sign that we should move things.

So let us go ahead and move loop out of main:

-- | Asks the user for a number until
--   they guessed the correct one.
guessNumber :: Integer -> IO ()
guessNumber secret = go 1 
 where
  go tries = do
     guess <- askNumberOrQuit
     case compare guess secret of
        LT -> putStrLn "Too small!" >> go (tries + 1)
        GT -> putStrLn "Too big!"   >> go (tries + 1)
        EQ -> printf "You found the number in %d tries!\n" tries

Using >> instead of do is a personal choice, but I like that both go (tries + 1) line up. YMMV.

This brings us to the following succinct main:

module Main (main) where

main :: IO ()
main = do
  printf "Find the number between %d and %d.\n" smallest biggest
  secret <- randomRIO (smallest, biggest) :: IO Integer
  guessNumber secret

Note that I've moved secret closer to its point of use. This motivates

main :: IO ()
main = do
  printf "Find the number between %d and %d.\n" smallest biggest

  randomRIO (smallest, biggest) >>= guessNumber

But again, that's a matter of choice and up to you. So what's the point of main being so small? Well, at the moment, there's no choice for the user to make the game harder or easier. Adding that to your "large" main above would make things messy. Adding this to the small main makes things not-messy:

-- | Returns a ordered pair from user input,
--   or default range if the user enters "default".
askForRangeOrDefault :: IO (Integer, Integer)

main :: IO ()
main = do
  putStrLn "Welcome to the number guesser!"
  (smallest, biggest) <- askForRangeOrDefault

  printf "Find the number between %d and %d.\n" smallest biggest
  randomRIO (smallest, biggest) >>= guessNumber

The nice part is that regardless of how you mess up main, guessNumber will still work. That's the great part of modularity.

\$\endgroup\$

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.