Please comment on my Hangman game

Learn
#1

Hello! I’m new to Haskell, coming from Dart, Python, and F#. I would very much appreciate it if some of you could offer constructive feedback on my very first Haskell program:

Thank you!

5 Likes
#2

Welcome to Haskell! Hope you’re enjoying the language :slight_smile:

I’ll get right to the reviewing:

  • Instead of doing safeHead <$> getLine, there’s also just getChar (though you might have to explicitly print a newline after it for esthetic reasons)
  • In the play function, you repeat the entire GameState _ _ _ a few times without changing anything. Would be easier to tell if you’d use an as-pattern like the following:
play gameState@(GameState secretWord goodGuesses badGuesses) = do
...
    AlreadyGuessed -> do
        showState gameState
        putStrLn "..."
        pure gameState
....

This way, it’s immediately obvious it’s just the state that we started with, AND there’s less clutter to read :wink:

  • The {-# LANGUAGE Strict #-} pragma probably doesn’t do much in this situation, but I’d warn against using this without knowing what it will actually do. A lot of Haskell is expecting lazyness, and making literally everything strict from the get-go will hurt you more than help you. StrictData is generally seen as not a bad idea (though in edge cases could also bite you, but that’s only when you want to explicitly use laziness, e.g. like having a field be undefined / error ".." until some processing is done, after which that field gets filled before it’s actually used)

I personally really like default laziness so I can put huge computations in a let binding that “might” be run in certain cases, without having to wonder if the location of that let binding is going to influence the evaluation of that huge computation or not. Because that can actually worsen performance.

All in all this is very pleasant code to read, and seems pretty solid for what it’s supposed to do, so I’d like to congratulate you on a first program well done :+1:

5 Likes
#3

I would use a record with named fields for GameState:

data GameState = GameState { 
     secretWord :: SecretWord,
     goodGuesses :: GoodGuesses,
     badGuesses ::  BadGuesses
   } deriving Show

and pattern-match like this:

getOutcome (GameState {secretWord, goodGuesses, badGuesses})

This is a minor thing, but using the named fields frees you from having to “invent” new binding names in each match, and from worrying about making them consistent between matches. Matching a single field is also easier this way.

Another thing you could try: you determine what pic to show by indexing into pics, the index being determined by the number of bad guesses. Instead of indexing, how about having a field called remainingPics in the GameState itself? That feels more direct to me. You would show the head of remainingPics, and whittle the list as the user makes wrong guesses.

1 Like
#4

Thank you for taking the time to review!

  • I tried getChar initially, and at this point, I’m 99% convinced that it’s broken on Windows. I struggled with getChar/buffering for over an hour before giving up and writing the safeHead solution. Where can I report this issue? Would it be here? Glasgow Haskell Compiler / GHC · GitLab

  • I didn’t know about as-patterns. I’ll try that!

  • Thank you for your advice about Strict. I know that using Strict in my very first program means I didn’t even give laziness a chance! Currently, I’m leaning toward continuing to use it because of this presentation: https://www.youtube.com/watch?v=22BdAEMtMOQ

Thanks again!

#5

Thank you for taking the time to review!

  • Yes, I will try record syntax.

  • I actually had a Tries counter initially, but then I realized I can just use the length of the badGuesses to keep track of the index.

Thanks again!

#6

I haven’t watched the entire presentation, but I’d like to add that the Strict pragma will only be applied to anything defined in the module in which it is set. You will still be encountering lots of laziness because of things like tuples and Maybe, since they are defined without the Strict pragma in the base library. And any function not defined in your module will probably also be lazy. (e.g. && and ||)

So while it may help a bit for programmers used to strict evaluation, do not expect your Haskell code in its entirety to suddenly be strict. As long as you realize this and accept this discrepancy, I wish you lots of Strict success :slight_smile:

1 Like
#7

Based on your suggestions, I’ve updated the game to use both as-patterns and records:

play :: GameState -> IO ()
play gameState@GameState {..} = do
    guess <- getGuess
    let outcome = getOutcome gameState guess
    case outcome of
        Lose -> do
            showState gameState {badGuesses = Set.insert guess badGuesses}
            putStrLn "\nYou lose!"
            putStrLn $ "The word was: " ++ secretWord
        Win -> do
            showState gameState {goodGuesses = Set.insert guess goodGuesses}
            putStrLn "\nYou win!"
        AlreadyGuessed -> do
            showState gameState
            putStrLn "\nYou already guessed that letter."
            play gameState
        GoodGuess -> do
            showState gameState {goodGuesses = Set.insert guess goodGuesses}
            putStrLn "\nCorrect."
            play gameState {goodGuesses = Set.insert guess goodGuesses}
        BadGuess -> do
            showState gameState{badGuesses = Set.insert guess badGuesses}
            putStrLn "\nIncorrect."
            play gameState {badGuesses = Set.insert guess badGuesses}
4 Likes
#8

Small improvement here could be the updating of the gameState in the GoodGuess and BadGuess cases, since you’re updating it twice to the same thing. You could do that once:

GoodGuess -> do
    let updatedState =
            gameState {goodGuesses = Set.insert guess goodGuesses}
    showState updatedState
    putStrLn "\nCorrect."
    play updatedState

But yeah, it looks a lot cleaner now :+1:

2 Likes