Ruby FizzBuzz solution with Rspec test

Question

My code makes the four tests pass, but I'm worried I'm missing something. Is the code too simple and not encompassing enough? I'm just beginning to learn so any feedback is welcome!

Here's the Rspec followed by my code; notice there's no 1-100 requirement like on some other FizzBuzz challenges:

describe "fizzbuzz" do
  it 'returns "Fizz" when the number is divisible by 3' do
fizz_3 = fizzbuzz(3)

expect(fizz_3).to eq("Fizz")
  end
  it 'returns "Buzz" when the number is divisible by 5' do
fizz_5 = fizzbuzz(5)

expect(fizz_5).to eq("Buzz")
  end
 it 'returns "FizzBuzz" when the number is divisible by 3 and 5' do
fizz_15 = fizzbuzz(15)

expect(fizz_15).to eq("FizzBuzz")
  end
  it 'returns nil when the number is not divisible by 3 or 5' do
fizz_4 = fizzbuzz(4)

    expect(fizz_4).to eq(nil)
  end
end

---

def fizzbuzz(int)
  if int % 3 == 0 && int % 5 == 0
   return "FizzBuzz"
  end

  if int % 5 == 0
   return "Buzz"
  end

  if int % 3 == 0
   return "Fizz"
  end

end

Answer 1

I'll just get this out the way: The method should work fine. The problems aren't with the logic.

Indentation and whitespace are... awful. Seriously, it's a total mess. Did multiple double-takes before I was able to make sense of where tests began and ended. The method itself is more readable, but still inconsistent. Considering it's so simple there's really no excuse for messy whitespace.

Second, don't call a variable by its value. What I mean is don't do this

fizz_3 = fizzbuzz(3)
expect(fizz_3).to eq("Fizz")

What if you wanted to check with the number 6 instead? Now the variable would have to change name too to make sense (in two places, even), which means it's not really variable, is it? Besides, for this you can just write

expect(fizzbuzz(x)).to eq y

and skip the variable.

Third, why return nil? As I said in a comment, a solution typically prints/returns either "Fizz", "Buzz", "FizzBuzz", or the number. That said, there's no "official" FizzBuzz specification (closest you find is probably this), because that's not the point. Similarly, there's no single "correct" solution, because that's not the point either. All FizzBuzz does is (firstly) test whether someone is able to solve the task at all, and (secondly) how they went about it.

• You solved it (mostly - or with some atypical requirements), which is good. But really, that's the ground floor here.

• There are tests, which is good. Not that the tests are important in and of themselves for something so trivial, but the ability to write tests is a plus.

• But the code is an almost unreadable mess, which is bad.

FizzBuzz was conceived as an interviewer's tool. Provided the person being interviewed is able to program at all, the solution provides something to examine and discuss.

So if I were to pretend to be an interviewer, I'd say thank you for your time, and show you the door. This is just too sloppy.

But assuming the code was the same, just using proper whitespace, I might ask the following questions (just concerning the method itself):

• How would you refactor this to avoid the duplication of logic? This is a practical concern.

• How would you refactor this to only use the method's implicit return? Having multiple explicit returns is not the most Ruby-like thing to do, so this is more stylistic concern.

• What might you use in place of the if statements? This is a general competence question.

And provided I'd given you the task without specifying what should be returned by default, I'd also ask:

• Why return nil? I want to hear the reasoning behind this, because it effective means the method can have two return types, meaning callers may have more work to do in checking the returned value. I want to know if you considered the method's usage.

Of course, this is CodeReview, not CodeInterview, and I don't expect answers to those questions. It's just food for thought - which is real purpose of FizzBuzz. For some inspiration, see this question.