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