Project Euler # 1: Sum of multiples of 3 or 5 less than 1000 in Haskell [closed]

Question

-- 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

Answer 1

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.