Array containing numbers from 1 to N

Question

I am trying to return an array containing the numbers from 1 to N, where N will never be less than 1, with the following conditions:

• If the value is a multiple of 3: use the value 'Fizz' instead.
• If the value is a multiple of 5: use the value 'Buzz' instead.
• If the value is a multiple of 3 and 5: use the value 'FizzBuzz' instead.

This is what I have right now. Am I going in the right direction?

def fizzbuzz(n)
  x = [1..n]
  x.map { |i|
    if (i % 3 == 0 && i % 5 == 0)
      'FizzBuzz'
    elsif (i % 3 == 0 && i % 5 != 0) 
      'Fizz'
    elsif (i % 5 == 0 && i % 3 != 0)
      'Buzz'
    end

  puts x
end

Answer 1

In such a short piece of code, you have so many crucial mistakes and bad habits.

1. [Bug] Your { is not closed.
2. [Bug] You have the wrong object [1..n].
3. [Bug] You are trying to create an array using map, but somehow you are not doing anything with it, and are instead doing puts on the original object. That is meaningless.
4. [Bug] You haven't given an appropriate value when neither of the condition is met.
5. [Refactor] Your condition is redundant. You only need to either put the (i % 3 == 0 && i % 5 == 0) condition at the beginning, or use the i % 5 != 0 and i % 3 != 0 conditions, but not both. The former is smarter.
6. [Refactor] You are using x only once, where chaining is easy. You should do chaining.
7. [Refactor] There is zero? method that you can use.
8. [Refactor] You had some unnecessary parentheses.
9. [Style] Since the conditioned value is short enough, you can put them each in one line.
10. [Style] It is pretty much established among Rubyists to use do ... end rather than { ... } when the block exceeds a single line.

Four bugs in thirteen lines is pretty much. A corrected code would be:

def fizzbuzz(n)
  (1..n).map do |i|
    if (i % 3).zero? && (i % 5).zero? then 'FizzBuzz'
    elsif (i % 3).zero? then               'Fizz'
    elsif (i % 5).zero? then               'Buzz'
    else                                   i
    end
  end
end
puts fizzbuzz(10)

Answer 2

You're in the right direction. The biggest issue is the array definition. The following:

x = [1..n]

… will create an array with a single value — a 1..n range object.

Any of these are valid alternatives:

x = 1..n          # no need for an array since you're using map
x = (1..n).to_a   # converts the range to an array explicitly
x = [*1..n]       # unsplats the range

The other big issues, as point out by @sawa, are an unclosed bracket and the use of map instead of map! (alternatively, use x = x.map { … } or simply return x.map { … } and move puts outside of the function).

Answer 3

Some code optimization, it is just additionals to sawa's answer:

def fizzbuzz(n)
   x = Array.new(n) { '' }.map.with_index(1) do |v,i|
      v << 'Fizz' if (i % 3).zero?
      v << 'Buzz' if (i % 5).zero?
      v.empty? && i || v
   end
end
fizzbuzz(15)
# => [1, 2, "Fizz", 4, "Buzz", "Fizz", 7, 8, "Fizz", "Buzz", 11, "Fizz", 13, 14, "FizzBuzz"] 

Comments:

1. Creating a new array filled with empty strings with nth dimension.
2. Since indexation begins from 1, incrementing to 1 in a loop.
3. Using three condition we set a proper new value for each one in the array.
4. The line v.empty? && i || v can be replace with v.empty? ? i : v, but it is for a taste. I prefer && || pair.

Answer 4

def fizzbuzz(n)
  [*1..n].each { |i|
      if i % 3 == 0 && i % 5 == 0
         'FizzBuzz'
      elsif  i % 3 == 0 
         'Fizz'
      elsif i % 5 == 0
         'Buzz'
      end
   }
end

You can use the case statements as well:

[*1..16].each { |i|
  case 
  when (i % 3).zero? && (i % 5).zero?
      ‘FizzBuzz'
  when  (i % 3).zero?
      'Fizz'
  when (i % 5).zero?
      'Buzz'
  end
}