Problem
I’m currently working on a simple command line application in Haskell and I have three functions that are quite repetitive.
The three functions:
getAllFilmsByDirector :: [Film] -> String -> String
getAllFilmsByDirector filmsDb directorName = unlines [name ++ ", " ++ director ++ ", " ++ show year ++ ", " ++ show (calculateRatings likes dislikes)|(name, director, year, likes, dislikes) <- filmsDb, director == directorName]
getAllFilmsWithHighRatings :: [Film] -> String
getAllFilmsWithHighRatings filmsDb = unlines [name ++ ", " ++ director ++ ", " ++ show year ++ ", " ++ show (calculateRatings likes dislikes)|(name, director, year, likes, dislikes) <- filmsDb, (calculateRatings likes dislikes) >= 75]
getAllFilmsByDirectorAvgRating :: [Film] -> String -> Float
getAllFilmsByDirectorAvgRating filmsDb directorName = roundTo (calculateAverageRating [calculateRatings likes dislikes|(name, director, year, likes, dislikes) <- filmsDb, director == directorName]) 1
Film Type
type Film = (String, String, Int, [String], [String])
testDatabase :: [Film]
testDatabase = [("Blade Runner", "Ridley Scott", 1982, ["Zoe", "Heidi", "Jo", "Kate", "Emma", "Liz", "Dave"], ["Sam", "Olga", "Tim"]), ("The Fly", "David Cronenberg", 1986, ["Garry", "Dave", "Zoe"], ["Kevin", "Emma", "Heidi", "Jo", "Kate"]), ("Body Of Lies", "Ridley Scott", 2008, ["Garry", "Dave"], ["Bill", "Olga", "Tim", "Zoe", "Paula"]), ("Avatar", "James Cameron", 2009, ["Dave", "Amy", "Liz"], ["Olga", "Tim", "Zoe", "Paula"]), ("Titanic", "James Cameron", 1997, ["Zoe", "Emma", "Paula", "Liz", "Olga", "Dave"], ["Sam", "Wally", "Kate"]), ("The Departed", "Martin Scorsese", 2006, ["Wally", "Liz", "Kevin", "Tim", "Emma"], ["Olga", "Dave", "Kate", "Zoe"]), ("Aliens", "Ridley Scott", 1986, ["Dave", "Garry", "Liz", "Sam", "Wally", "Kate", "Zoe"], ["Tim", "Emma", "Jo", "Olga"]), ("Kingdom Of Heaven", "Ridley Scott", 2005, ["Jo", "Wally", "Emma"], ["Tim", "Garry", "Ian", "Neal"]), ("Alien: Covenant", "Ridley Scott", 2017, ["Kevin", "Tim"], ["Emma", "Jo", "Liz"]), ("E.T. The Extra-Terrestrial", "Steven Spielberg", 1982, ["Dave", "Amy", "Garry", "Ian", "Neal"], ["Jenny", "Kate", "Emma", "Olga"]), ("Bridge of Spies", "Steven Spielberg", 2015, ["Wally", "Sam", "Dave", "Neal"], ["Bill", "Garry", "Ian", "Kate"]), ("Jaws", "Steven Spielberg", 1975, ["Jenny", "Emma", "Bill", "Neal"], ["Sam", "Ian", "Kate"]), ("The Martian", "Ridley Scott", 2015, ["Wally", "Sam", "Dave", "Jo", "Jenny", "Kate", "Emma", "Olga"], ["Ian", "Neal", "Tim", "Liz"]), ("The BFG", "Steven Spielberg", 2016, ["Sam", "Wally", "Dave", "Jo", "Kate"], ["Neal"]), ("The Shawshank Redemption", "Frank Darabont", 1994, ["Dave", "Amy", "Bill", "Garry", "Ian", "Neal", "Kate", "Jenny", "Zoe", "Heidi"], ["Jo"]), ("Gladiator", "Ridley Scott", 2000, ["Olga", "Neal", "Kate", "Garry"], ["Heidi", "Bill", "Sam", "Zoe"]), ("The Green Mile", "Frank Darabont", 1999, ["Kevin", "Tim", "Emma", "Heidi"], ["Kate", "Jenny", "Zoe"]), ("True Lies", "James Cameron", 1994, ["Sam", "Dave"], ["Emma", "Olga", "Jenny", "Zoe"]), ("Super 8", "J J Abrams", 2011, ["Kevin", "Tim", "Emma", "Olga", "Heidi"], ["Wally", "Dave", "Jenny", "Zoe"]), ("Minority Report", "Steven Spielberg", 2002, ["Kevin", "Kate", "Tim", "Emma", "Jenny", "Zoe"], ["Olga", "Heidi"]), ("War Horse", "Steven Spielberg", 2011, ["Garry", "Bill", "Olga", "Jo", "Wally", "Emma", "Tim", "Kate", "Zoe"], ["Heidi", "Jenny", "Sam"]), ("Silence", "Martin Scorsese", 2016, ["Wally", "Emma", "Tim", "Heidi", "Bill", "Jo"], ["Dave", "Olga"]), ("The Terminal", "Steven Spielberg", 2004, ["Kate", "Dave", "Jo", "Wally", "Emma"], ["Heidi"]), ("Star Wars: The Force Awakens", "J J Abrams", 2015, ["Emma", "Wally", "Zoe", "Kate", "Bill", "Dave", "Liz"], ["Olga", "Jo", "Neal"]), ("Hugo", "Martin Scorsese", 2011, ["Wally", "Sam"], ["Kate", "Bill", "Dave"])]
roundTo
roundTo x n = (fromIntegral (round (x * 10^n))) / 10^n
calculateRatings
calculateRatings :: [String] -> [String] -> Int
calculateRatings likes dislikes = round(fromIntegral (length likes) / fromIntegral (length likes + length dislikes) * 100)
calculateAvgRating
calculateAverageRating :: [Int] -> Float
calculateAverageRating rating = fromIntegral (round(fromIntegral ((foldr (+) 0 rating)) / fromIntegral (length rating) * 100)) / 100
I’m fairly new to Haskell and its syntax but I know I should probably make use of the getAllFilmsByDirector
in the getAllFilmsByDirectorAvgRating
however because the getAllFilmsByDirectorAvgRating
takes a Film
type rather than a string, I’m not too sure about the best way to modify the code to make use of the getFilmsByDirector
function.
I’m also using very similar list comprehensions in each of the three and if anyone has any ideas on how I can make these less repetitive that would be amazing.
Solution
Don’t repeat yourself
Your code suffers from being WET. Your Film
‘s formatting function is repeated. So let us get rid of that first:
prettyFilm :: Film -> String
prettyFilm (name, director, year, likes, dislikes) =
name ++ ", " ++ director ++ ", " ++ show year ++ ", " ++ show rating
where rating = calculateRatings likes dislikes
Now getAllFilmsByDirector
and getAllFilmsWithHighRatings
are slightly easier to read:
getAllFilmsByDirector :: [Film] -> String -> String
getAllFilmsByDirector filmsDb directorName =
unlines [prettyFilm film | film@(_, director, _, _, _) <- filmsDb
, director == directorName]
getAllFilmsWithHighRatings :: [Film] -> String
getAllFilmsWithHighRatings filmsDb =
unlines [prettyFilm film | film@(_, _, _, likes, dislikes) <- filmsDb
, calculateRatings likes dislikes >= 75]
However, that still looks like there is of duplicate code: in the end both functions filter the filmsDb
and then print all films. If we had getFilmsBy :: (Film -> Bool) -> [Film] -> [Film]
we could write:
getAllFilmsByDirector :: [Film] -> String -> String
getAllFilmsByDirector filmsDb directorName = unlines . map prettyFilm . getFilmsBy p $ filmsDb
where
p (_, _, director, _, _) = director == directorName
getAllFilmsWithHighRatings :: [Film] -> String
getAllFilmsWithHighRatings filmsDb = unlines . map prettyFilm . getFilmsBy p $ filmsDb
where
p (_, _, _, likes, dislikes) = calculateRatings likes dislikes >= 75
Luckily, getFilmsBy = filter
.
Now that we got rid of the list comprehensions, we can see that we can still improve our functions if we extract unlines . map prettyFilm . filter
:
showAllFilmsBy :: (Film -> Bool) -> [Film] -> String
showAllFilmsBy p = unlines . map prettyFilm . filter p
getAllFilmsByDirector :: [Film] -> String -> String
getAllFilmsByDirector filmsDb directorName = showAllFilmsBy p filmsDb
where
p (_, _, director, _, _) = director == directorName
getAllFilmsWithHighRatings :: [Film] -> String
getAllFilmsWithHighRatings filmsDb = showAllFilmsBy p filmsDb
where
p (_, _, _, likes, dislikes) = calculateRatings likes dislikes >= 75
Other review items
- Some of your parentheses are superfluous, for example those in
roundTo
. Try to get rid of those. - Almost all your functions have type signatures (good!), except for
roundTo
(bad). Try to add type signatures to all top-level bindings. - Unless you really need to use
Double
instead ofFloat
. - Try to stay in the original type
Film
as long as possible. You can always go fromFilm
toString
, but the inverse isn’t true. That’s why you couldn’t reuse your code.
Try to put configuration-like parameters to the left and data-like parameters to the right, it composes better.
genericLength
condenses much conversion.
ImplicitParams
lets me improvise a query language so I don’t need to unpack the tuple everywhere.
You already wrote roundTo
and then you didn’t use it.
{-# LANGUAGE ImplicitParams #-}
getAllFilmsByDirector :: String -> [Film] -> String
getAllFilmsByDirector director = unlines . selectwhere ?pretty (?director == director)
getAllFilmsWithHighRatings :: [Film] -> String
getAllFilmsWithHighRatings = unlines . selectwhere ?pretty (?rating >= 0.75)
getAllFilmsByDirectorAvgRating :: String -> [Film] -> Float
getAllFilmsByDirectorAvgRating director
= roundTo 1 . liftA2 (/) sum genericLength . selectwhere ?rating (?director == director)
-- selectwhere :: Query a -> Query Bool -> [Film] -> [a]
selectwhere select cond = map (exec select) . filter (exec cond) where
-- exec :: Query a -> Film -> a
exec query (name, director, year, likes, dislikes) = let
?rating = roundTo 2 $ genericLength likes / (genericLength likes + genericLength dislikes)
?pretty = name ++ ", " ++ director ++ ", " ++ show year ++ ", " ++ show ?ratings
?director = director
in query
roundTo n x = fromIntegral (round (x * 10^n)) / 10^n