[Haskell-beginners] Critique my program

[Roel van Dijk]

I realize this is 2 weeks old but I have a few small comments.

> getShipsFromFile :: String -> IO ([ShipPlacement])

The parenthesis around [ShipPlacement] are superfluous.

>hitShip :: [ShipPlacement] -> BoardLocation -> Maybe ShipClass
>hitShip ships loc = case (find (isLocationInShip loc) ships) of
>    Just (ShipPlacement cls _ _) -> Just cls
>    Nothing                      -> Nothing

Also unnecessary parenthesis around the case argument.

And there is a function hidden in your case expression. "find
(isLocationInShip loc) shipsFind" returns a value of type (Maybe
ShipPlacement). Your case converts it to a (Maybe ShipClass). So the
type of the hidden function is (Maybe ShipPlacement -> Maybe
ShipClass). Now look at the type of the fmap function: fmap :: Functor
f => f a -> f b. The last piece of this little puzzle is a function
that converts a ShipPlacement to a ShipClass. Luckily you have already
written it: the field 'placeShip' of the ShipPlacement record.

Maybe has an instance for Functor. This means you can
write the hitShip function like this:

> hitShip :: [ShipPlacement] -> BoardLocation -> Maybe ShipClass
> hitShip ships loc = fmap placeShip $ find (isLocationInShip loc) ships

Next I noticed how your validateShips function checks for erroneous
conditions in a very specific order. First you check for invalid
placements and then for overlapping ships. If the order of these
checks is not important you can write with a bit less plumbing using
the some functions from Data.Maybe:

> validateShips :: [ShipPlacement] -> Maybe GameError
> validateShips ships =
>     listToMaybe $ catMaybes [ validatePlacements ships
>                             , validateOverlap    ships
>                             ]

Hmm, not sure if that is an improvement :-) It would be if there where
more checks.

> isValidPlacement :: ShipPlacement -> Bool
> isValidPlacement (ShipPlacement ship (BoardLocation row col) Vertical) =
>     (col >= 1 && col <= boardWidth) &&
>     (row >= 1 && row <= boardHeight - (shipSize ship - 1))
> isValidPlacement (ShipPlacement ship (BoardLocation col row) Horizontal) =
>     (col >= 1 && col <= boardWidth - (shipSize ship - 1)) &&
>     (row >= 1 && row <= boardHeight)

There is a lot of common code in these 2 equations. This can be made
mode explicit:

> isValidPlacement :: ShipPlacement -> Bool
> isValidPlacement (ShipPlacement ship (BoardLocation row col) o) =
>     case o of
>       Vertical   -> check 0 shipLength
>       Horizontal -> check shipLength 0
>   where
>     check x y = col >= 1 && col <= boardWidth - x &&
>                 row >= 1 && row <= boardHeight - y
>     shipLength = shipSize ship - 1

In general I noticed that you sometimes refer to a value of
ShipPlacement as a 'ship' and sometimes as a 'placement'. I think it
would be a bit easier to read if you are consistent in the naming of
variables.

Overall I would say the quality of the code is not bad at all. It is
easy to follow the flow of the program and the module structure is
sensible.

Regards,
Roel