0
\$\begingroup\$

Project Euler problem #1:

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

My code is correct, but I don't know how I can optimize and make that faster.

N = gets.to_i
N.times do 
  arr = []
  (0...gets.to_i).each { |j| arr << j if j % 3 == 0 || j % 5 == 0 }
  puts arr.reduce(:+)
end
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Compare codereview.stackexchange.com/a/26594/35991. \$\endgroup\$ – Martin R Nov 24 '14 at 6:35
  • 1
    \$\begingroup\$ I'm confused about your notation. What is N — the number mentioned in your challenge, or the number of times to repeat the challenge? \$\endgroup\$ – 200_success Nov 24 '14 at 6:56
  • \$\begingroup\$ Better known as Euler problem #1. \$\endgroup\$ – Mast Dec 16 '14 at 8:07
3
\$\begingroup\$

You use a constant N as your variable... not dreadful, but a little confusing.

You first ask the user for how many times they want to run the code, then for which number to sum the mods of 3 & 5 for; it would be simpler to just write the code which asks for a number and works the sum - if the user wants to run it multiple times, then let them! :-)

No need to start at zero - 0,1, & 2 are never going to be divisible by three (although we'd need to add some validation in case the user adds a very low number)

Instead of setting up an intermediate arr value and iterating with each, you can select to get the numbers that return true for the block.

But your main question is about speed - have you benchmarked your code to find out if the speed is an issue?

n = gets.to_i
return 0 unless n > 2 # you could do this as a ternary or `if` statement if you prefer
(3...n).select { |j| j % 3 == 0 || j % 5 == 0 }.reduce(:+)
\$\endgroup\$
2
  • \$\begingroup\$ Wouldn't it be better to skip building a temporary with select; i.e., just (3...n).reduce....? \$\endgroup\$ – Cary Swoveland Nov 28 '14 at 1:14
  • \$\begingroup\$ Then it would need to be something like: (3...n).reduce(0) { |sum, j| j % 3 == 0 || j % 5 == 0 ? sum+= j : sum } - I think that's less clear to decipher what it's doing \$\endgroup\$ – Pavling Nov 28 '14 at 17:16
2
\$\begingroup\$

If you want to write idiomatic Ruby code, I'd recommend Pavling's answer. It's far more readable and obvious what's going on.

You can make this perform better by doing less things. You're doing one thing that's entirely unnecessary: storing each number in an Array. It is much more efficient to skip this step and just sum the numbers. For a little extra efficiency, use a while loop instead of an Enumerator.

n = gets.to_i
j = 3
r = 0

while j < n
  r += j if j % 3 == 0 || j % 5 == 0
  j += 1
end

puts r

Now all we're doing is arithmetic.

\$\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.