A couple of months ago, I was tutoring my cousin in an introductory computer science class. We talked about his assignment in which he had to implement a subset of the Battleship board game using Java. I decided it would be fun for me to try to complete the assignment in Haskell. I forgot about the project for a while, but I picked it up and finished it today. I have put the code on BitBucket (a Mercurial based code hosting site for those who don't know) at this location: http://bitbucket.org/dfrey/battleship/ I am hoping that some people from this list could review my code. I'm not very concerned with correctness. I'm more interested in how my code can be clarified through use of existing functions or standard Haskell idioms. If you don't have Mercurial installed or don't want to clone the repository, you can browse the source here: http://bitbucket.org/dfrey/battleship/src/ The original PDF from my cousin's assignment can be viewed here: http://bitbucket.org/dfrey/battleship/raw/81a01574f164/Assn4.pdf Feel free to submit your comments in any format. Patches, written comments, complete re-writes of the program, etc. are all welcome. Thanks, David Frey
On Sunday 10. January 2010 20.44.38 David Frey wrote:
I am hoping that some people from this list could review my code. I'm not very concerned with correctness. I'm more interested in how my code can be clarified through use of existing functions or standard Haskell idioms.
A great start is – in my experience – is to use hlint on a program. Running hlint v1.6.14 on the code finds 26 suggestions of which 3 are classified as errors. While you don't necessarily want to follow every suggestion it comes up with, it is still a great way to find code that can be improved. If you have cabal-install, you can install hlint by running “cabal install hlint” and then run “hlint *.hs”. -- Erlend Hamberg “Everything will be ok in the end. If its not ok, its not the end.” GPG/PGP: 0xAD3BCF19 45C3 E2E7 86CA ADB7 8DAD 51E7 3A1A F085 AD3B CF19
Am Sonntag 10 Januar 2010 20:44:38 schrieb David Frey:
A couple of months ago, I was tutoring my cousin in an introductory computer science class. We talked about his assignment in which he had to implement a subset of the Battleship board game using Java.
I decided it would be fun for me to try to complete the assignment in Haskell. I forgot about the project for a while, but I picked it up and finished it today.
I have put the code on BitBucket (a Mercurial based code hosting site for those who don't know) at this location: http://bitbucket.org/dfrey/battleship/
I am hoping that some people from this list could review my code. I'm not very concerned with correctness. I'm more interested in how my code can be clarified through use of existing functions or standard Haskell idioms.
I prefer to have e.g. data BoardLocation = BoardLocation { locationRow :: Int , locationCol :: Int } deriving (Show, Eq) but that's a matter of taste. One thing that I dislike is unsignedInteger = many1 digit >>= \x -> return $ read x better is unsignedInteger = many1 digit >>= return . read , but m >>= return . f isn't good, that should be fmap f m or liftM f m , so I'd recommend unsignedInteger = fmap read (many1 digit) or, using Control.Applicative, unsignedInteger = read <$> many1 digit signedInteger could be signedInteger = do signFun <- option id (char '-' >> return negate) fmap signFun unsignedInteger it might be good to be more lenient with the format and allow more than one space between items (and trailing whitespace for user input). allPairs (x:xs) = [(x,y) | y <- xs] ++ allPairs xs
If you don't have Mercurial installed or don't want to clone the repository, you can browse the source here: http://bitbucket.org/dfrey/battleship/src/
The original PDF from my cousin's assignment can be viewed here: http://bitbucket.org/dfrey/battleship/raw/81a01574f164/Assn4.pdf
Feel free to submit your comments in any format. Patches, written comments, complete re-writes of the program, etc. are all welcome.
Thanks, David Frey
I have received a couple of responses off list so far. One came from Erlend Hamberg. He suggested that I run hlint as a first step. The other response came from Daniel Fischer. He provided more concise versions of some of the functions that I wrote. I have made changes based on both responses and committed it to http://bitbucket.org/dfrey/battleship/ Thanks for the suggestions. David On January 10, 2010 11:44:38 am David Frey wrote:
A couple of months ago, I was tutoring my cousin in an introductory computer science class. We talked about his assignment in which he had to implement a subset of the Battleship board game using Java.
I decided it would be fun for me to try to complete the assignment in Haskell. I forgot about the project for a while, but I picked it up and finished it today.
I have put the code on BitBucket (a Mercurial based code hosting site for those who don't know) at this location: http://bitbucket.org/dfrey/battleship/
I am hoping that some people from this list could review my code. I'm not very concerned with correctness. I'm more interested in how my code can be clarified through use of existing functions or standard Haskell idioms.
If you don't have Mercurial installed or don't want to clone the repository, you can browse the source here: http://bitbucket.org/dfrey/battleship/src/
The original PDF from my cousin's assignment can be viewed here: http://bitbucket.org/dfrey/battleship/raw/81a01574f164/Assn4.pdf
Feel free to submit your comments in any format. Patches, written comments, complete re-writes of the program, etc. are all welcome.
Thanks, David Frey
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
participants (4)
-
Daniel Fischer -
David Frey -
Erlend Hamberg -
Roel van Dijk