HiveBrain v1.2.0
Get Started
← Back to all entries
patternrubyModerate

Multiples of 3 and 5 from Euler code challenge

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
challengecodemultipleseulerandfrom

Problem

I have implemented the first coding challenge on http://projecteuler.net/. I would love to get it reviewed by some expert rubyists!

# Multiples of 3 and 5
#
# If we list all of the natural numbers below 10 that are multiples of
# 3 and 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.

class Multiples

  def multiples
    numbers = Array(1..999)
    multiples = Array.new
    for i in numbers
      if i%3 == 0 or i%5 == 0
        multiples.push(i)
      end
    end
    multiples
  end

  def sumMultiples(multiples)
    total = 0
    multiples.each { |i| total+= i }
    puts(total)
  end

end

multiples = Multiples.new
multiples.sumMultiples(multiples.multiples)

Solution

-
Don't use or. It's for flow control. Use || for boolean logic.

-
Don't use Array.new, use []

-
Don't use (1..999).to_a or Array(1..999) when all you really want to do is iterate. It's expensive to instantiate a 1000 item array when really you just want to iterate from 1 to 999

-
Any time you find yourself declaring an empty array and pushing items onto it in a loop, you are probably missing a map or select:

def multiples
  numbers = Array(1..999)
  multiples = Array.new
  for i in numbers
    if i%3 == 0 or i%5 == 0
      multiples.push(i)
    end
  end
  multiples
end


Could be better written this way:

def multiples
  (1..999).select do |i|
    i % 3 == 0 || i % 5 == 0
  end
end


Your summation loop follows a similar pattern:

def sumMultiples(multiples)
  total = 0
  multiples.each { |i| total+= i }
  puts(total)
end


Again, there is a more idiomatic way of doing this via inject:

def sumMultiples(multiples)
  total = multiples.inject(&:+)
end


A breakdown of how this works can be found in How to sum array of numbers in Ruby?.

Code Snippets

def multiples
  numbers = Array(1..999)
  multiples = Array.new
  for i in numbers
    if i%3 == 0 or i%5 == 0
      multiples.push(i)
    end
  end
  multiples
end
def multiples
  (1..999).select do |i|
    i % 3 == 0 || i % 5 == 0
  end
end
def sumMultiples(multiples)
  total = 0
  multiples.each { |i| total+= i }
  puts(total)
end
def sumMultiples(multiples)
  total = multiples.inject(&:+)
end

Context

StackExchange Code Review Q#26573, answer score: 11

Revisions (0)

No revisions yet.