rmurphey / js-assessment-answers

125 stars 84 forks source link

Fizz-buzz test solutions are misleading #27

Closed jamesplease closed 9 years ago

jamesplease commented 9 years ago

https://github.com/rmurphey/js-assessment/pull/95 demonstrates out a potential area of confusion in the fizzbuzz solutions, which #26 attempts to address.

The issue is that the modulo operator returns 0 when it's not divisible, which evaluates to false in the if statement. The solutions, therefore, test the opposite of what one would expect.

To describe it another way, when num % 5 is true, the expression evaluates to false.

26 does a good job at trying to clarify this with words, but @ashleygwilliams proposed a code change that makes the code itself more expressive.

The idea would just be to do something like:

      // divisible by 3 but not 5
      if (num % 3 === 0 && num % 5 !== 0) {
        return 'fizz';
      }

      // divisible by 5 but not 3
      if (num % 5 === 0 && num % 3 !== 0) {
        return 'buzz';
      }

It may even be simpler by just having one expression in each if. I'm not sure, 'cuz I don't have the tests open. But, you know, the idea is to use === to make the code more explicit.

Either this solution or #26 would close this issue. At the moment, I can't say which proposal I prefer!

francesmcmullin commented 9 years ago

I prefer the change described here - I think it's better to change the code making it more explicit rather than adding comments.

ashleygwilliams commented 9 years ago

thanks for your feedback @davecocoa !!

jamesplease commented 9 years ago

@ashleygwilliams want to whip up a PR implementing the explicit === operators, or would you like me to give it a try?

ashleygwilliams commented 9 years ago

@jmeas i've got it, i'll try to get it done today.

jamesplease commented 9 years ago

Sounds good!