Skip to main content
Code Review

Return to Answer

deleted 5 characters in body; deleted 2 characters in body
Source Link
200_success
  • 145.5k
  • 22
  • 190
  • 478

Another example of procedural thinking is in the name checkFull. A better name would be checkWinnerisFull.

get :: Int -> Board -> Tile
get pos (Board board)
 | pos < 1 || pos > 9 = Nothing
 | otherwise = board !! (pos - 1)
 
put :: Player -> Int -> Board -> Maybe Board
put player pos (Board board)
 | pos < 1 || pos > 9 = Nothing
 | isNothing $ get pos (Board board) = Just newBoard
 | otherwise = Nothing
 where
 update i (x:xs) y
 | i == 0 = y:xs
 | otherwise = x : update (i - 1) xs y
 newBoard = Just $ Board $ update (pos - 1) board $ Just player
rows :: Board -> [[Tile]]
rows (Board b) = chunk 3 b 
 where
 -- Or use chunk from Data.List.Split
 chunk _ [] = []
 chunk n xs = (take n xs) : (chunk n $ drop n xs)

Another example of procedural thinking is in the name checkFull. A better name would be checkWinner.

get :: Int -> Board -> Tile
get pos (Board board)
 | pos < 1 || pos > 9 = Nothing
 | otherwise = board !! (pos - 1)
 
put :: Player -> Int -> Board -> Maybe Board
put player pos (Board board)
 | pos < 1 || pos > 9 = Nothing
 | isNothing $ get pos (Board board) = newBoard
 | otherwise = Nothing
 where
 update i (x:xs) y
 | i == 0 = y:xs
 | otherwise = x : update (i - 1) xs y
 newBoard = Just $ Board $ update (pos - 1) board $ Just player
rows :: Board -> [[Tile]]
rows (Board b) = chunk 3 b 
 where
 -- Or use chunk from Data.List.Split
 chunk _ [] = []
 chunk n xs = (take n xs) : (chunk n $ drop n xs)

Another example of procedural thinking is in the name checkFull. A better name would be isFull.

get :: Int -> Board -> Tile
get pos (Board board)
 | pos < 1 || pos > 9 = Nothing
 | otherwise = board !! (pos - 1)
 
put :: Player -> Int -> Board -> Maybe Board
put player pos (Board board)
 | pos < 1 || pos > 9 = Nothing
 | isNothing $ get pos (Board board) = Just newBoard
 | otherwise = Nothing
 where
 update i (x:xs) y
 | i == 0 = y:xs
 | otherwise = x : update (i - 1) xs y
 newBoard = Board $ update (pos - 1) board $ Just player
rows :: Board -> [[Tile]]
rows (Board b) = chunk 3 b 
 where
 -- Or use chunk from Data.List.Split
 chunk _ [] = []
 chunk n xs = (take n xs) : (chunk n $ drop n xs)
deleted 288 characters in body
Source Link
200_success
  • 145.5k
  • 22
  • 190
  • 478
winner :: Board -> Maybe Player 
winner (Board board) = winner' $ streaks board
 where
 allSame [_] = True
 allSame (x:y:zs) = (x == y) && allSame (y:zs)
 
 streaks b = map (map (b !!)) [
 [0, 1, 2], [3, 4, 5], [6, 7, 8], -- rows
 [0, 3, 6], [1, 4, 7], [2, 5, 8], -- columns
 [0, 4, 8], [2, 4, 6] ] -- diagonals
 
 winner' xs 
 | isNothing w = Nothing
 | otherwise = head $ fromJust $ find allSame xsw
 where w = find allSame xs
main = do
 putStrLn "This is classic tic tac toe game."
 putStrLn "In order to play, you need to put a number between 01 and 8"9"
 putStrLn "This table shows tile numbers"
 putStrLn $ tileGuide
 putStrLn ""
 putStrLn $ show $ emptyBoard
 playGame $ emptyBoard
 where
 playGame b = do
 playerchoice <- prompt "Make a move: "
 let newboard = put Player1 (read playerchoice) b
 case newboard of
 Nothing -> do
 putStrLn "Invalid move."
 playGame b
 Just b' ->
 case winner b' of
 Just Player1 -> do
 putStrLn $ show b'
 putStrLn "You win!"
 _ -> if isFull b' then
 putStrLn "Tie"
 else do
 b'' <- compMove b'
 putStrLn $ show b''
 case winner b'' of
 Just Player2 -> putStrLn "You Lose!"
 _ -> if isFull b'' then
 putStrLn "Tie"
 else
 playGame b''
winner :: Board -> Maybe Player 
winner (Board board) = winner' $ streaks board
 where
 allSame [_] = True
 allSame (x:y:zs) = (x == y) && allSame (y:zs)
 
 streaks b = map (map (b !!)) [
 [0, 1, 2], [3, 4, 5], [6, 7, 8], -- rows
 [0, 3, 6], [1, 4, 7], [2, 5, 8], -- columns
 [0, 4, 8], [2, 4, 6] ] -- diagonals
 
 winner' xs 
 | isNothing w = Nothing
 | otherwise = head $ fromJust $ find allSame xs
 where w = find allSame xs
main = do
 putStrLn "This is classic tic tac toe game."
 putStrLn "In order to play, you need to put a number between 0 and 8"
 putStrLn "This table shows tile numbers"
 putStrLn $ tileGuide
 putStrLn ""
 putStrLn $ show $ emptyBoard
 playGame $ emptyBoard
 where
 playGame b = do
 playerchoice <- prompt "Make a move: "
 let newboard = put Player1 (read playerchoice) b
 case newboard of
 Nothing -> do
 putStrLn "Invalid move."
 playGame b
 Just b' ->
 case winner b' of
 Just Player1 -> do
 putStrLn $ show b'
 putStrLn "You win!"
 _ -> if isFull b' then
 putStrLn "Tie"
 else do
 b'' <- compMove b'
 putStrLn $ show b''
 case winner b'' of
 Just Player2 -> putStrLn "You Lose!"
 _ -> if isFull b'' then
 putStrLn "Tie"
 else
 playGame b''
winner :: Board -> Maybe Player 
winner (Board board) = winner' $ streaks board
 where
 allSame [_] = True
 allSame (x:y:zs) = (x == y) && allSame (y:zs)
 
 streaks b = map (map (b !!)) [
 [0, 1, 2], [3, 4, 5], [6, 7, 8], -- rows
 [0, 3, 6], [1, 4, 7], [2, 5, 8], -- columns
 [0, 4, 8], [2, 4, 6] ] -- diagonals
 
 winner' xs 
 | isNothing w = Nothing
 | otherwise = head $ fromJust w
 where w = find allSame xs
main = do
 putStrLn "This is classic tic tac toe game."
 putStrLn "In order to play, you need to put a number between 1 and 9"
 putStrLn "This table shows tile numbers"
 putStrLn $ tileGuide
 putStrLn ""
 putStrLn $ show $ emptyBoard
 playGame $ emptyBoard
 where
 playGame b = do
 playerchoice <- prompt "Make a move: "
 let newboard = put Player1 (read playerchoice) b
 case newboard of
 Nothing -> do
 putStrLn "Invalid move."
 playGame b
 Just b' ->
 case winner b' of
 Just Player1 -> do
 putStrLn $ show b'
 putStrLn "You win!"
 _ -> if isFull b' then
 putStrLn "Tie"
 else do
 b'' <- compMove b'
 putStrLn $ show b''
 case winner b'' of
 Just Player2 -> putStrLn "You Lose!"
 _ -> if isFull b'' then
 putStrLn "Tie"
 else
 playGame b''
Source Link
200_success
  • 145.5k
  • 22
  • 190
  • 478

User experience

When someone wins, print the winning board configuration before exiting. Just accepting the input and printing "You win!" leaves an unsatisfactory feeling.

Regardless of how it is represented internally, for the user interface, the board should be indexed in a way that mimics a computer number keypad:

7 8 9
4 5 6
1 2 3

Implementation

There's a lot of brute-force enumeration of all the cases. I suppose a 3 ×ばつ 3 board is small enough to allow that.


data Tile and data Player feel redundant. I would just define type Tile = Maybe Player. In addition, Player should derive Eq (and possibly Show as well).

data Player = Player1 | Player2 deriving (Eq, Show)
type Tile = Maybe Player
tile :: Tile -> String
tile (Just Player1) = "X"
tile (Just Player2) = "O"
tile Nothing = " "

A few of your functions are too procedural. For example, instead of printBoard :: Board -> IO(), it would be better to make Board an instance of Show, such that show board produces a String.

data Board = Board [Tile]
instance Show Board where
 show (Board b) = intercalate "\n" $ map ((extercalate "|") . (map tile)) $ reverse $ rows (Board b)
 where extercalate delim list = delim ++ (intercalate delim list) ++ delim
emptyBoard :: Board
emptyBoard = Board $ replicate (size^2) Nothing
 where size = 3
-- indexing scheme for get and put
tileGuide :: String
tileGuide =
 intercalate "\n" ["|7|8|9|",
 "|4|5|6|",
 "|1|2|3|"]

Another example of procedural thinking is in the name checkFull. A better name would be checkWinner.

isFull :: Board -> Bool
isFull (Board b) = not $ any isNothing b

There's a bit of asymmetry in the names getTile and put. Personally, I would just choose get and put.

For put, the board parameter should go last. That lets you write a chained expression like fromJust $ put Player2 3 $ fromJust $ put Player1 6 emptyBoard. In your original form, such an expression would be nasty.

get :: Int -> Board -> Tile
get pos (Board board)
 | pos < 1 || pos > 9 = Nothing
 | otherwise = board !! (pos - 1)
 
put :: Player -> Int -> Board -> Maybe Board
put player pos (Board board)
 | pos < 1 || pos > 9 = Nothing
 | isNothing $ get pos (Board board) = newBoard
 | otherwise = Nothing
 where
 update i (x:xs) y
 | i == 0 = y:xs
 | otherwise = x : update (i - 1) xs y
 newBoard = Just $ Board $ update (pos - 1) board $ Just player
rows :: Board -> [[Tile]]
rows (Board b) = chunk 3 b 
 where
 -- Or use chunk from Data.List.Split
 chunk _ [] = []
 chunk n xs = (take n xs) : (chunk n $ drop n xs)

As with checkFullisFull, you should rename checkWinner to winner.

Something suspicious happened with your copy-and-paste code: the cases for O are in a weird order.

With a list representation, you can use !! indexing.

winner :: Board -> Maybe Player 
winner (Board board) = winner' $ streaks board
 where
 allSame [_] = True
 allSame (x:y:zs) = (x == y) && allSame (y:zs)
 
 streaks b = map (map (b !!)) [
 [0, 1, 2], [3, 4, 5], [6, 7, 8], -- rows
 [0, 3, 6], [1, 4, 7], [2, 5, 8], -- columns
 [0, 4, 8], [2, 4, 6] ] -- diagonals
 
 winner' xs 
 | isNothing w = Nothing
 | otherwise = head $ fromJust $ find allSame xs
 where w = find allSame xs

To finish, here are the additional imports to make the code above work:

import Data.List (find, intercalate)
import Data.Maybe (fromJust, isNothing)

... and some minor modifications to main:

main = do
 putStrLn "This is classic tic tac toe game."
 putStrLn "In order to play, you need to put a number between 0 and 8"
 putStrLn "This table shows tile numbers"
 putStrLn $ tileGuide
 putStrLn ""
 putStrLn $ show $ emptyBoard
 playGame $ emptyBoard
 where
 playGame b = do
 playerchoice <- prompt "Make a move: "
 let newboard = put Player1 (read playerchoice) b
 case newboard of
 Nothing -> do
 putStrLn "Invalid move."
 playGame b
 Just b' ->
 case winner b' of
 Just Player1 -> do
 putStrLn $ show b'
 putStrLn "You win!"
 _ -> if isFull b' then
 putStrLn "Tie"
 else do
 b'' <- compMove b'
 putStrLn $ show b''
 case winner b'' of
 Just Player2 -> putStrLn "You Lose!"
 _ -> if isFull b'' then
 putStrLn "Tie"
 else
 playGame b''
lang-hs

AltStyle によって変換されたページ (->オリジナル) /