2
\$\begingroup\$

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
\$\endgroup\$
  • 1
    \$\begingroup\$ Er.. typically, a fizzbuzz solution should return (or print, if you're printing directly) the integer as a last resort - not nil \$\endgroup\$ – Flambino Jul 17 '16 at 23:14
2
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ Thank you for your answer. To be clear, the only part of this code I wrote was the second code box. The entire Rspec was given to me as the problem, I didn't write it at all so I can't answer why it was formatted the way it was. I also don't know why they wanted me to return nil, because again, I only wrote from "def fizzbuzz(int)" on down. I included the Rspec only to show what tests they wanted my code to pass. \$\endgroup\$ – BrooksDavis Jul 18 '16 at 5:03
  • 1
    \$\begingroup\$ @BrooksDavis Ah, didn't know the tests were given to you. Please tell whomever you got them from that it's very, very poor code (honestly, to use this for teaching is terrible). Anyway, you too have some inconsistent whitespace in your method, though nowhere near as bad. And as also mentioned there's a bit of logic duplication, but other than that, you solved the task, which is the only real goal in FizzBuzz, so kudos. But there are many, many ways of writing it, so do check out the link at the end of my answer too. \$\endgroup\$ – Flambino Jul 18 '16 at 6:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.