I am starting to learn Haskell, and following Dr. Erik Meijer lectures online, he suggests as an exercise to implement the game of Nim in Haskell, so I implemented a really basic game for two players (no computer player).

I can imagine that this code is far from being good Haskell, and I accept all possible criticism and suggestions about it (if you can link me to other lectures/talks/guidelines, I appreciate it), but there are some aspects that I would love to ask you:

1. The comments: I am not really sure how am I supposed to document the code in Haskell, so I just tried to explain each of the methods before its declaration. If there is another convention, please let me know.

2. The parenthesis: Should I use more the $ or . operators than parenthesis? I kind of struggle with trying to use composition with functions when there is more than one argument. 3. The nested do blocks: I fixed an error doing it, but it feels incredibly hacky and I don't really understand why the compiler is not able to understand the code when there is more than one line inside an if clause (doesn't matter which one, both were problematic). If I don't put the do keyword, the compiler was interpreting that putStrLine was receiving 4 arguments: its real argument and the next function call (turn board player, for instance). Why did it happen? This is the code itself: import qualified Data.Sequence as Seq import qualified Data.Foldable as Fol import qualified Data.List as List import Data.Maybe -- Player type is used to take control of the turns and who wins data Player = One | Two deriving (Show, Eq) -- Change player returns the player that is NOT the one passed as argument change :: Player -> Player change One = Two change Two = One -- Board is an alias for a Sequence of Ints, as they allow us to update single -- elements in easily. type Board = Seq.Seq Int -- This is the initial board structure initialBoard :: Board initialBoard = Seq.fromList [5, 4, 3, 2, 1] -- The move method checks if the a movement can be executed and returns the -- updated board in case it is possible move :: Board -> (Int, Int) -> Maybe Board move board (row, stars) | and [(Seq.index board row) >= stars, row < 5] = Just (Seq.adjust (\x -> x - stars) row board) | otherwise = Nothing -- The display methods transforms a Board into a nice, enumerated String of -- asterisks display :: Board -> String display board = List.intercalate "\n" (zipWith (++) numbers (stars board)) where numbers = ["1. ", "2. ", "3. ", "4. ", "5. "] stars board = [(concat . take n) (repeat "* ") | n <- Fol.toList board] -- The next methods are the ones that control IO main :: IO () main = nim -- Main method welcomes the player, displays the initial board and calls the -- first turn nim :: IO () nim = do putStrLn "Welcome to nim!" putStrLn (display initialBoard) turn initialBoard One -- The turn method displays the player and asks for a movement, then checks if -- there was a problem performing that movement and continues the game. This is -- the main game loop turn :: Board -> Player -> IO () turn board player = do putStrLn ("\nIt's your turn, Player " ++ (show player) ++ "!") putStrLn "Choose a row to remove stars!" row <- getLine putStrLn "How many stars do you want to remove?" stars <- getLine let newBoard = move board ((read row) - 1, read stars) if newBoard == Nothing then do putStrLn "Not valid movement" turn board player else isOver (fromJust newBoard) (change player) -- isOver checks if the Board is empty, and checks whether the game is over or -- the next turn must be called isOver :: Board -> Player -> IO() isOver board player = do if board == Seq.fromList [0, 0, 0, 0, 0] then putStrLn ("Congratulations, Player " ++ (show player) ++ ", you win!") else do putStrLn "" putStrLn (display board) turn board player  1 Answer 1. Haskell generates rich documentation from specially marked up comments using Haddock. If you're finding it difficult to write a description that does more than restate the type give some usage examples. E.g., >>> change One Two  Then consider future proofing your examples with doctest. 2. This is fine, don't worry about it. 3. (This explanation trades rigor for accessibility. Hopefully.) The contents of the then and else blocks aren't part of the overall turn do-block. The if statement returns whichever of then or else based on the given predicate, which in your case must be an IO monad action, but in the general case of ifs doesn't have to be. While if/then/else is syntax, consider this function definition which is equivalent and not syntax. boolCase :: Bool -> a -> a -> a boolCase True a _ = a boolCase False _ b = b  So your broken if statement was the same as— boolCase (newBoard == Nothing) (putStrLn "Not valid movement" turn board player) (isOver (fromJust newBoard) (changePlayer))  Now that it looks like a normal function call do you see the problem with the second argument to boolCase? Without explicitly using do you are calling putStrLn with four arguments as that line break is no longer meaningful. This is the same as when you use if/then/else. Just because you are using an if in a do-block doesn't mean that the contents of then and else are in that same do-block. Use type aliases to disambiguate repeated types that would be inconvenient to newtype. This improves your documentation without needing to write boring comments too! type Row = Int type Stars = Int  Don't tuple your arguments to functions unnecessarily. E.g., use move :: Board -> Row -> Stars -> Maybe Board instead of what you have now. move has an error in that it checks the number of stars in a row before it checks that the row is in bounds. move is kinda hairy with that multi-line guard. Here's an alternate version (one of many possible!) that takes advantage of the Alternative instance of Maybe (and should take advantage of the new ApplicativeDo extension if you enable it!). import Control.Monad (guard) move :: Board -> Row -> Stars -> Maybe Board move board row stars = do guard$ 0 <= row && row < 5
guard $stars <= board Seq.index row pure$ Seq.adjust (subtract stars) row board


Here's an alternate version of display too. Take advantage of pre-existing functions with semantically meaningful names!

import Data.List (intersperse, replicate)

display :: Board -> String
display board = unlines \$ zipWith (++) numbers (stars board)
where
numbers = ["1. ", "2. ", "3. ", "4. ", "5. "]
stars = map (intersperse ' ' . flip replicate '*') . toList


Your game play and display logic is smeared across nim, turn and isOver. Try extracting the pure components into reusable, nameable functions (like the proposition in isOver). Instead of if and then fromJust, use a case statement (case newBoard of Nothing -> ...; Just b -> ...). Use a safe version of read (Text.Read.readMaybe) and deal with errors properly.

You might also want to use Data.Array instead of Data.Sequence for constant time indexing.

With practice and familiarity I think you'll do fine! This is really very good for a beginner.

• In addition to what Rowan said I would also try to eliminate the explicit recursion: Instead of calling turn/isOver recursively from each other, return the new arguments back to the caller, which will then handle passing the data on. That way, you can understand all data flow just by reading the caller. You can then use the library Control.Monad.Loops to try and find a common pattern that you can apply in the caller in place of recursion. – Gurkenglas Jun 23 '16 at 12:19
• Thank you both! I am learning functional programming on my own, so this feedback is incredibly useful. – Diego Vicente Jun 23 '16 at 17:30