FizzBuzz with user input variables

Question

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";
}
?>

Answer 1

I know you've been seeing a lot of php, but I think this is the first time I've seen you ask a question with the sole intention of learning php! 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.

Answer 2

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.