7
\$\begingroup\$

This my first program in Haskell. I'm organizing a ping-pong tournament and I want to calculate a tournament table automatically. The table should contain scores in all games and some statistics (points, wins, losses, balls, ...). The input is hardcoded directly in the Haskell file.

import Data.Tuple


players = ["John", "Bob", "David", "Alice"]

type Score = (Int, Int)
type Game = ((String, String), [Score])

games :: [Game]
games = [(("John", "Bob"), [(11, 7), (11, 8), (3, 11)]),
         (("John", "David"), [(11, 0), (11, 3)]),
         (("David", "Alice"), [(9, 11), (9, 11), (8, 11)]),
         (("Alice", "John"), [(11, 0), (11, 0)])]

validScore :: Score -> Bool
validScore (left, right) = (left == 11 && right < left - 1) ||
                           (left > 11 && right == left - 2) ||
                           (right == 11 && left < right - 1) ||
                           (right > 11 && left == right - 2)

winner :: Score -> Bool
winner (left, right) | validScore (left, right) = left > right

wins :: [Score] -> Int
wins xs = length (filter winner xs)

losses :: [Score] -> Int
losses xs = length xs - length (filter winner xs)

gamePoints :: [Score] -> Int
gamePoints xs | wins xs > losses xs = 1
              | wins xs < losses xs = 0

swapGame :: Game -> Game
swapGame game = (swap (fst game), map swap (snd game))

findGame :: String -> String -> [Game]
findGame player opponent = [game | game <- games, fst (fst game) == player && snd (fst game) == opponent] ++
                           [swapGame game | game <- games, fst (fst game) == opponent && snd (fst game) == player]

playerGames :: String -> [[Game]]
playerGames player = map (findGame player) players

calculate :: ([Score] -> Int) -> String -> Int
calculate f player = sum [f (snd game) | game <- concat (playerGames player) ]

playerPoints = calculate gamePoints
playerGamesCount = calculate (const 1)
playerWins = playerPoints
playerLosses = calculate (\scores -> 1 - gamePoints scores)
playerGoals = calculate (\scores -> sum (map fst scores))
playerMisses = calculate (\scores -> sum (map snd scores))
playerDiff = calculate (\scores -> sum (map fst scores) - sum (map snd scores))

showScore :: Score -> String
showScore (left, right) = show left ++ ":" ++ show right

showScores :: [Score] -> String
showScores [x] = showScore x
showScores (x : xs) = showScore x ++ "," ++ showScores xs

showGames :: [[Game]] -> String
showGames [[]] = " "
showGames [[(_, scores)]] = showScores scores
showGames ([] : games) = "  | " ++ showGames games
showGames ([(_, scores)] : games) = showScores scores ++ " | " ++ showGames games


data Row = Row String [[Game]] Int Int Int Int Int Int Int

instance Show Row where
    show (Row player games gamesCount wins losses goals misses diff points) =
        show player ++ " | " ++
        showGames games ++ " | " ++
        show gamesCount ++ " | " ++
        show wins ++ " | " ++
        show losses ++ " | " ++
        show goals ++ " | " ++
        show misses ++ " | " ++
        show diff ++ " | " ++
        show points

playerRow player = Row player
                   (playerGames player)
                   (playerGamesCount player)
                   (playerWins player)
                   (playerLosses player)
                   (playerGoals player)
                   (playerMisses player)
                   (playerDiff player)
                   (playerPoints player)

table :: [Row]
table = map playerRow players


main = mapM_ (\row -> putStrLn (show row)) table

Output:

"John" |   | 11:7,11:8,3:11 | 11:0,11:3 | 0:11,0:11 | 3 | 2 | 1 | 47 | 51 | -4 | 2
"Bob" | 7:11,8:11,11:3 |   |   |   | 1 | 0 | 1 | 26 | 25 | 1 | 0
"David" | 0:11,3:11 |   |   | 9:11,9:11,8:11 | 2 | 0 | 2 | 29 | 55 | -26 | 0
"Alice" | 11:0,11:0 |   | 11:9,11:9,11:8 |   | 2 | 2 | 0 | 55 | 26 | 29 | 2

Is there something in the code that you would change? I'm not very interested in the correctness of the program, but in the usage of basic Haskell features and syntax constructs. Can something be written more compact?

\$\endgroup\$
5
\$\begingroup\$

I will try to do some review.

wins xs = length (filter winner xs)
=>
wins = length . filter winner

And the same with losses and others:

losses xs = length xs - length (filter winner xs)
=>
losses = length . filter (not . winner)

Manual string concatenation may be simplified:

showScores [x] = showScore x
showScores (x : xs) = showScore x ++ "," ++ showScores xs
=>
showScores = intercalate "," $ map showScore

In some cases forM_ is more readable:

main = mapM_ (\row -> putStrLn (show row)) table
=>
main = forM_ table (\row -> putStrLn (show row))

Or even:

=>
main = mapM_ (putStrLn . show) table
\$\endgroup\$
  • 1
    \$\begingroup\$ putStrLn . show is print \$\endgroup\$ – Squidly Dec 12 '14 at 16:40
  • \$\begingroup\$ @VladimirParfinenko Please disregard my earlier comment. It has been pointed out to me that this is actually a good Haskell review, pardon my unfamiliarity. Welcome to Code Review! \$\endgroup\$ – Phrancis Dec 12 '14 at 20:01
1
\$\begingroup\$

Something that tickled me is

winner :: Score -> Bool

because the function name and its output are conflicting. It's like I ask you: "Who is the winner of this set ?" And you answer me: "True !"

I can think of two different to make it better:

  • Rename the function to match what it does:

    leftPlayerWins :: Score -> Bool
    
  • Create a type PlayerSide to tell where is the player:

    data PlayerSide = LeftSide | RightSide
    

    And your function becomes

    winner :: Score -> PlayerSide
    winner (left, right)
        | validScore (left, right) = if left > right 
                                     then LeftSide
                                     else RightSide
    

Another thing: your functions are really tied to the data structure you use (tuples), to the point the function findGame is barely readable because of the multiple fst/snd! That problem can be (partially) solve just by clarifying your intent when you access data.

getPlayer :: PlayerSide -> (a,a) -> a
getPlayer side = case side of
                     LeftSide  -> fst
                     RightSide -> snd

(Notice how side is the first parameter to make the function easier to use with map/filter)

-- or --

leftPlayer :: (a,a) -> a
leftPlayer = fst

rightPlayer :: (a,a) -> a
rightPlayer = snd

Note about the types : I decided to use a tuple (a,a) instead of a Score because the functions are usable to get an Int from a Score or to get a String from the first tuple in the Game type. I think it is reasonable in this context but you may want to differentiate the two cases in a bigger program.

Now we abstract a bit the functions around Game:

playersName :: Game -> (String, String)
playersName game = fst game

getScores :: Game -> [Score]
getScores game = snd game

Combined with a let ... in statement, your function findGame is now way clearer:

findGame :: String -> String -> [Game]
findGame player opponent = [game | game <- games, 
                                   let players = playersName game
                                       leftPlayer' = leftPlayer players
                                       rightPlayer' = rightPlayer players
                                   in (player == leftPlayer' && opponent == rightPlayer')
                                   || (player == rightPlayer' && opponent == leftPlayer')]

(No more need for this swapGame function !)

If you want to exercise, the best would be to rewrite playerPoints, ..., playerDiff with the new functions!

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.