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!
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!
Welcome to Haskell! Hope you’re enjoying the language
I’ll get right to the reviewing:
safeHead <$> getLine
, there’s also just getChar
(though you might have to explicitly print a newline after it for esthetic reasons)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
{-# 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
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.
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!
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!
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
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}
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