7
\$\begingroup\$

Yes, another FizzBuzz! I know you guys may be tired of these but I think it's a tradition stepping stone for beginners on this site.

Here are some notes on my thinking:

  • I tried to keep it as flexible as possible where the logic is deducted from the variables as much as possible. So one could potentially make a user input form on a website and let the user select all the values (numbers and words), and the code just does it.

  • I tried to avoid magic numbers as much as I know how. Not sure if 0 counts as a magic number.

  • I did not feel the need to add any comments as I feel it is self-explanatory. If you feel otherwise, please let me know.

Here is a link to PhpFiddle for your convenience.

<?php
$counter = 1;
$stopper = 100;
$fizzWord = 'Fizz';
$fizzNumber = 3;
$buzzWord = 'Buzz';
$buzzNumber = 5;

for ($counter = 1; $counter <= $stopper; $counter++) {
    if ((($counter %($fizzNumber * $buzzNumber) == 0))) {
        echo $fizzWord . $buzzWord;
    }
    elseif (($counter % $fizzNumber) == 0) {
        echo $fizzWord;
    }
    elseif (($counter % $buzzNumber) == 0) {
        echo $buzzWord;
    }   
    else {
        echo $counter;
    }
echo "<br>\n";
}
?>
\$\endgroup\$
  • 1
    \$\begingroup\$ I don't think you put the right link to the fiddle. \$\endgroup\$ – RubberDuck Sep 23 '14 at 22:22
  • \$\begingroup\$ PhpFiddle has been acting wonky for me. I meant to link to a saved query but it is not letting me save it, so I linked to the blank query page... \$\endgroup\$ – Phrancis Sep 24 '14 at 0:33
7
\$\begingroup\$

I know you've been seeing a lot of , but I think this is the first time I've seen you ask a question with the sole intention of learning ! Great :)

I think you'd be able to handle any critique thrown at you, at so I'll do my best, but it's a simple script which leaves little room for huge critiques!

  • The variable name $counter is ambiguous, I feel. I keep wanting to assume it's some Counter object! A more suitable name in my opinion may be $index or a synonym to that!
  • $stopper is also a strange name. It's seems too friendly for the code. What about $lastIndex or $endPoint or even $stoppingIndex.
  • It's just my opinion, but I would find it easier to handle the trigger words in arrays. Such that we may have:

    $triggerWords = [
        ["divisor" => 3, "word" => "Fizz"],
        ["divisor" => 5, "word" => "Buzz"]
    ]
    

    This would open up the possibility of expanding the amount of trigger words we want, plus it's more modularized, plus it's easier to change values in the future. You won't be restricted to two triggers, specifically "fizz" and "buzz", either. This will take a loop inside your main loop so it can iterate the array, but since you didn't mention optimization, this shouldn't be an issue.

  • Setting $counter as the first argument in the for loop overwrites what you set on the first line. This could just be a mistake, and that's fine, just making you aware is all.
  • Regarding your algorithm: it looks very clunky and over-complicated. Take into consideration rolfl's answer, and perhaps look up other implementations of PHP FizzBuzz (or Java, it's similar looking to PHP and is easy to read if you don't know the language!). It does look like you have one or two syntax errors (extra parentheses), and you may want to run this on multiple sites to make sure you are given the same result.
  • Lastly, you need spaces/indentation before your last echo! Best to keep things leveled on their scope.

Very nice, good work.

\$\endgroup\$
  • 1
    \$\begingroup\$ Hey! It's good to see an @AlexL answer again! \$\endgroup\$ – RubberDuck Sep 24 '14 at 0:36
  • \$\begingroup\$ TBH it never dawned on me to use an array, because I did not know how (or why) so I shall look into them :) The parentheses did confused me a bit, still getting used to counting parentheses to make sure it is right. Do you think any kind of vertical whitespace is justified in stuff like that? \$\endgroup\$ – Phrancis Sep 24 '14 at 0:41
  • \$\begingroup\$ When I use any IDE, one of the first keyboard shortcuts I learn first (if it has it), is the auto-format command. I think having correct indentation and syntax is incredibly important! \$\endgroup\$ – Alex L Sep 24 '14 at 1:15
10
\$\begingroup\$

If your intention is to let users adjust the values used for the fizz and buzz values (3, and 5), then it is possible that they may choose numbers that break your logic quite badly. For example, if they chose '3' and '3', then we would expect all multiples of 3 to be 'FizzBuzz', but, instead, only multiples of 9 would be 'FizzBuzz'. This is because you need to find the lowest common multiple (LCM) of the fizz and buzz values, not just the product of them.

Either you need to calculate the LCM, or you can do them independently using a double test like:

if ($counter % $fizzNumber == 0 && $counter % $buzzNumber == 0) {
    echo $fizzWord . $buzzWord;
....

Notice how I have also removed the redundant (...) braces you had around the conditions. The % operator has a higher precedence than == so it does not need to be elevated.

\$\endgroup\$

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.