Ruby: find multiples of 3 and 5 up to n. Can't figure out what's wrong with my code. Advic-CodePudding

I have been attempting the test below on codewars. I am relatively new to coding and will look for more appropriate solutions as well as asking you for feedback on my code. I have written the solution at the bottom and for the life of me cannot understand what is missing as the resultant figure is always 0. I'd very much appreciate feedback on my code for the problem and not just giving your best solution to the problem. Although both would be much appreciated. Thank you in advance!

The test posed is:

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.

Finish the solution so that it returns the sum of all the multiples of 3 or 5 below the number passed in. Additionally, if the number is negative, return 0 (for languages that do have them).

Note: If the number is a multiple of both 3 and 5, only count it once.

My code is as follows:

  array = [1..number]
  multiples = []
  if number < 0
    return 0
  else array.each{|x|
    if x % 3 == 0 || x % 5 == 0
      multiples << x
    end
    }
  end
  return multiples.sum
end

CodePudding user response：

In a situation like this, when something in your code produces an unexpected result you should debug it, meaning, run it line by line with the same argument and see what each variable holds. Using some kind of interactive console for running code (like irb) is very helpfull. Moving to your example, let's start from the beginning:

number = 10
array = [1..number]
puts array.size # => 1 - wait what?
puts array[0].class # => Range

As you can see the array variable doesn't contain numbers but rather a Range object. After you finish filtering the array the result is an empty array that sums to 0.
Regardless of that, Ruby has a lot of built-in methods that can help you accomplish the same problem typing fewer words, for example:

multiples_of_3_and_5 = array.select { |number| number % 3 == 0 || number % 5 == 0 }

When writing a multiline block of code, prefer the do, end syntax, for example:

array.each do |x|
  if x % 3 == 0 || x % 5 == 0
    multiples << x
  end
end

CodePudding user response：

I'm not suggesting that this is the best approach per se, but using your specific code, you could fix the MAIN problem by editing the first line of your code in one of 2 ways:

By either converting your range to an array. Something like this would do the trick:

array = (1..number).to_a

or by just using a range INSTEAD of an array like so:

range = 1..number

The latter solution inserted into your code might look like this:

number = 17

range = 1..number
  multiples = []
  if number < 0
    return 0
  else range.each{|x|
    if x % 3 == 0 || x % 5 == 0
      multiples << x
    end
    }
  end
  multiples.sum

#=>  60

CodePudding user response：

The statement return followed by end suggests that you were writing a method, but the def statement is missing. I believe that should be

def tot_sum(number, array)
  multiples = []
  if number < 0
    return 0
  else array.each{|x|
    if x % 3 == 0 || x % 5 == 0
      multiples << x
    end
    }
  end
  return multiples.sum
end

As you point out, however, this double-counts numbers that are multiples of 15.

Let me suggest a more efficient way of writing that. First consider the sum of numbers that are multiples of 3 that do not exceed a given number n.

Suppose

n = 3
m = 16

then the total of numbers that are multiples of three that do not exceed 16 can be computed as follows:

3 * 1   3 * 2   3 * 3   3 * 4   3 * 5
  = 3 * (1   2   3   4   5)
  = 3 * 5 * (1   5)/2
  = 45

This makes use of the fact that 5 * (1 5)/2 equals the sum of an algebraic series: (1 2 3 4 5).

We may write a helper method to compute this sum for any number n, with m being the number that multiples of n cannot exceed:

def tot_sum(n, m)
  p = m/n
  n * p * (1   p)/2
end

For example,

tot_sum(3, 16)
  #=> 45

We may now write a method that gives the desired result (remembering that we need to account for the fact that multiples of 15 are multiples of both 3 and 5):

def tot(m)
  tot_sum(3, m)   tot_sum(5, m) - tot_sum(15, m)
end

tot(   9) #=>       23
tot(  16) #=>       60
tot(9999) #=> 23331668