HiveBrain v1.2.0
Get Started
← Back to all entries
patternMinor

Basic Nim game in Haskell

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
gamenimbasichaskell

Problem

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:

-
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.

-
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.

-
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]

-- T

Solution

-
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.

-
This is fine, don't worry about it.

-
(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.

Code Snippets

>>> change One
Two
boolCase :: Bool -> a -> a -> a
boolCase True  a _ = a
boolCase False _ b = b
boolCase (newBoard == Nothing)
         (putStrLn "Not valid movement"
          turn board player)
         (isOver (fromJust newBoard) (changePlayer))
type Row = Int
type Stars = Int
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

Context

StackExchange Code Review Q#132641, answer score: 3

Revisions (0)

No revisions yet.