patternrubyModerate
Multiples of 3 and 5 from Euler code challenge
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
-
Don't use
-
Don't use
-
Any time you find yourself declaring an empty array and pushing items onto it in a loop, you are probably missing a
Could be better written this way:
Your summation loop follows a similar pattern:
Again, there is a more idiomatic way of doing this via
A breakdown of how this works can be found in How to sum array of numbers in Ruby?.
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
endCould be better written this way:
def multiples
(1..999).select do |i|
i % 3 == 0 || i % 5 == 0
end
endYour summation loop follows a similar pattern:
def sumMultiples(multiples)
total = 0
multiples.each { |i| total+= i }
puts(total)
endAgain, there is a more idiomatic way of doing this via
inject:def sumMultiples(multiples)
total = multiples.inject(&:+)
endA 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
enddef multiples
(1..999).select do |i|
i % 3 == 0 || i % 5 == 0
end
enddef sumMultiples(multiples)
total = 0
multiples.each { |i| total+= i }
puts(total)
enddef sumMultiples(multiples)
total = multiples.inject(&:+)
endContext
StackExchange Code Review Q#26573, answer score: 11
Revisions (0)
No revisions yet.