2
\$\begingroup\$

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)
\$\endgroup\$
1
  • \$\begingroup\$ 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. \$\endgroup\$ – mdfst13 Oct 26 '16 at 5:25
3
\$\begingroup\$

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
\$\endgroup\$
2
  • \$\begingroup\$ nice answer. you can remove 3 chars with reduce(:+) \$\endgroup\$ – Jonah Oct 29 '16 at 22:53
  • \$\begingroup\$ 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. \$\endgroup\$ – tokland Oct 30 '16 at 7:16

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.