-- Project Euler problem: 1
-- Note: GHCi implementaion: type 'main' to output result
-- Multiples of 3 and 5
{- Problem:
If we list all the natural numbers below 10 that are multiples of 3 or 5, we get 3, 5, 6 and 9. The sum of these multiples is 23.
Find the sum of all the multiples of 3 or 5 below 1000.
-}
-- all multiples of 3 less than 1000
multOfThree = [3*x | x <- [1..333] ]
-- all multiples of 5 less than 1000
multOfFive = [5*x | x <- [1..199] ]
-- common values of the two lists
commonValues = [ x | x <- multOfThree , elem x multOfFive]
-- list of all multiples, subtract [add negative of] commonValues (as there are two copies - one per list)
multiplesOfThreeOrFive = (multOfThree ++ multOfFive) ++ (map ((-1)*) commonValues)
sumOfMultiples = sum multiplesOfThreeOrFive
-- display sum:
main = print sumOfMultiples
-
4\$\begingroup\$ Welcome to StackExchange Code Review! Please review How do I ask a good Question? Specifically, it is best to explain what the code does. Also it would good if you could describe what you are hoping for from a review. \$\endgroup\$ – Stephen Rauch Apr 21 '17 at 19:21
Please don't take any of this personal.
-- all multiples of 3 less than 1000
multOfThree = [3*x | x <- [1..333] ]
You need a comment for this, because the spec(fication) say 1000 and you use 333. Way too complicated. I can divide 1000 by 3 in my head, but not 2345 by 13. Suggestions:
multOfThree = takeWhile (<1000) [3*x | x <- [1..]]
multOfFive = [5,10 .. 999]
The spec says 1000, but I wrote 999, so there is a slight downside to the second example.
You can completely skip the commonValues trickery with
import Data.List (union)
multiplesOfThreeOrFive = multOfThree `union` multOfFive
There is multiples
and sometimes the abbreviation mult
. This becomes annoying in larger code bases. Aim for consistency.
This ++ (map ((-1)*) commonValues
looks contrived to avoid set difference. Actually, there is something good about it: list difference and union have complexity O(n*m), i.e. for every element in the first list the entire second list must be scanned. This idea has only O(n+m).
What do you expect from a code review? Since you omitted that, here is what I deem a beautiful solution: Rewrite the spec:
Print the sum of the multiples below 1000 of 3 or 5.
I changed "find" to "print", omitted the example, and chose an ugly word order. Nothing that needs a brain or can be disputed. This specification can be translated directly into code:
divides x y = x `rem` y == 0
print (sum ([x | x<-[1..1000], divides x 3 || divides x 5 ]))
You might also write [x | x<-[1..1000], any (divides x) [3,5]]
, which lets you expand the list of divisors more easily.
You'll appreciate short, concise code that is close to the spec.