Array containing numbers from 1 to N

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

---

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

[Bug] Your { is not closed.

[Bug] You have the wrong object [1..n] .

[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.

[Bug] You haven't given an appropriate value when neither of the condition is met.

[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.

[Refactor] You are using x only once, where chaining is easy. You should do chaining.

[Refactor] There is zero? method that you can use.

[Refactor] You had some unnecessary parentheses.

[Style] Since the conditioned value is short enough, you can put them each in one line.

[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)

---

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).

---

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:

Creating a new array filled with empty strings with n th dimension.

Since indexation begins from 1, incrementing to 1 in a loop.

Using three condition we set a proper new value for each one in the array.

The line v.empty? && i || v v.empty? && i || v v.empty? && i || v can be replace with v.empty? ? i : v v.empty? ? i : v v.empty? ? i : v , but it is for a taste. I prefer && || pair.

链接地址: http://www.djcxy.com/p/25656.html

上一篇: FizzBu​​zz不会使用case / when发出嘶嘶声或嗡嗡声

下一篇: 包含从1到N的数字的数组