-3
\$\begingroup\$
-- 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
\$\endgroup\$

closed as unclear what you're asking by Mast, t3chb0t, Jamal Apr 22 '17 at 17:31

Please clarify your specific problem or add additional details to highlight exactly what you need. As it's currently written, it’s hard to tell exactly what you're asking. See the How to Ask page for help clarifying this question. If this question can be reworded to fit the rules in the help center, please edit the question.

  • 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
1
\$\begingroup\$

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.

\$\endgroup\$

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