Project Euler #1: multiples of 3 and 5 Ruby

Question

Total newbie here and wouldn't mind learning from Rubyists to look at how I tried to solve this challenge.

I started with 2 variables n which starts at 0 and total which is an array where I intend to push n if it satisfy the condition set by the 2 modulo. Using the until loop with a max of 1000, my intention was to change total from an empty array with values divisible by 3 and 5.

With total now with array items, I defined a method to take in total as a parameter that I can iterate to add values to a new method variable - sum. The method then returns the new value of sum.

Here's my code.

n = 0
total = []
until n >=1000 do 
 if n % 3 == 0 || n % 5 == 0
 total << n
 end
 n += 1
 total
end
def sumTotal(total)
 sum = 0
 total.each { |i| sum+= i.to_i}
 puts sum
end
sumTotal(total)

• mdfst13: Hi. Welcome to Code Review! This would benefit from a more precise definition of the problem being solved. See this question for an example. Also, it would save a little time if you mentioned that this was Project Euler #1. But good work on giving the title in words. That's also helpful.

Answer 1

About your code, a first refactor would include:

• n = 0 + until n >=1000 do -> 0.upto(999).each do |n| or (0...1000).each do |n|

• total.each { |i| sum += i.to_i}. Why to_i? it's already an integer.

A second refactor would, well, throw away the entire code :) That's how you'd write it in a low-level language (C, for example) but it's not idiomatic Ruby. In Ruby, you use functional constructs with enumerables. It's more concise, more readable, more declarative:

result = (0...1000).select { |n| n % 3 == 0 || n % 5 == 0 }.reduce(0, :+)

That would be perfectly fine but, in the context of the euler project, where you will use the same abstractions over and over, you can do better. What could you abstract here? I'd say: Integer#divisible_by? and Enumerable#sum. Once you've added these methods, the solution can be read out loud (write 0.upto(999) if you want an even more human-readable solution, even though people tend to prefer ranges):

result = (0...1000).select { |n| n.divisible_by?(3) || n.divisible_by?(5) }.sum

• Jonah: nice answer. you can remove 3 chars with reduce(:+)
• tokland: Thanks. Indeed, we could writereduce(:+)in this particular case, but, as a general rule, it's safer to always call inject with the identity value, in case the input collection is empty. Imagine that the new specs say "numbers between 256 and 258", we updated the range and now result is.. nil, while the correct answer is zero.