Fizz Buzz interview question code

Question

Reading Jeff Atwood's "Why Can't Programmers.. Program?" led me to think that almost all of the solutions would not be something I would want in my own code-base. The Fizz Buzz program specification from Imran is here which says:

Write a program that prints the numbers from 1 to 100. But for multiples of three print “Fizz” instead of the number and for the multiples of five print “Buzz”. For numbers which are multiples of both three and five print “FizzBuzz”.

Implementing my solution differently than almost everyone else - I would like to put my solution up for review (as everyone doing it differently must think that my solution is not the best).

Could you please review my approach. Is it overkill to suggest such a solution? Under what conditions would you split it into a Model and Views? Are there any issues with the code?

For a start, I don't think a language agnostic definition of the problem can fully describe the best solution. In an interview I would find out by asking questions what was expected of the Fizz Bang code. Probable questions and my assumed answers are:

1. Whether any changes were likely to be made in the future? (such as new words, different rules other than 'Fizz' on multiples of 3 and 'Bang' on 5).    Yes

2. Whether the existing code-base was procedural or OO?    OO

I have decided to split the model and view components so that different output formats could be easily implemented.

Model:

class Model_Fizz_Buzz
{
   private $fizzBuzz;

   /** Construct the Fizz Buzz game model.
    *  @param fizzBuzz \Array The list of period => text for the game.  If the
    *  number is a multiple of the period then the text should be used.
    */
   public function __construct(Array $fizzBuzz=array())
   {
      $this->fizzBuzz = $fizzBuzz;
   }

   /** Get the Fizz_Buzz game numbers.
    *  @param start \int The number to start from (defaults to 1).
    *  @param end   \int The number to finish with (defaults to 100).
    */
   public function get($start=1, $end=100)
   {
      $data = array();

      for ($num = $start; $num <= $end; $num++)
      {
         $data[$num] = '';

         foreach ($this->fizzBuzz as $period => $text)
         {
            if ($num % $period === 0)
            {
               $data[$num] .= $text;
            }
         }

         if (empty($data[$num]))
         {
            $data[$num] = $num;
         }
      }

      return $data;
   }
}

View:

class View_Text_Lines
{
   /** Write the data values separated by newlines.
    *  @param data \array The data to be written.
    */
   public function write(Array $data)
   {
      foreach ($data as $val)
      {
         echo $val . PHP_EOL;
      }
   }
}

Usage would be:

$fizzView = new View_Text_Lines();

echo PHP_EOL . '-- Standard Fizz Buzz' . PHP_EOL;
$fizzBuzz = new Model_Fizz_Buzz(array(3 => 'Fizz',
                                      5 => 'Buzz'));
$fizzView->write($fizzBuzz->get());

echo PHP_EOL . '-- Three Buzz Bang' . PHP_EOL;
$threeBuzz = new Model_Fizz_Buzz(array(3 => 'Three',
                                       4 => 'Buzz',
                                       5 => 'Bang'));
$fizzView->write($threeBuzz->get(30, 61));

Answer 1

Is it overkill to suggest such a solution?

Yes, I think so :-)

Under what conditions would you split it into a Model and Views?

If you need more than one view. For example, if the specification says that the program should be able to write the results to the screen, a CSV/PDF file, a network socket, a web service etc.

Are there any issues with the code?

It's fine, just three issues:

• Naming: I'd rename $data to $result.
• Input checks:
  • What should happen when $start > $end? (Check and throw an exception.)
  • What should happen when $period is not a number? (Throw an exception in the constructor.)
• Unit tests are missing.

Whether any changes were likely to be made in the future?

I would not try predicting it. If there are some change requests refactor it. The tests will show whether there is a regression or not. On Programmers.SE there are same questions about it:

• Design for future changes or solve the problem at hand
• Future proofing code

Answer 2

I agree that your solution is proper and any reviewer would understand that you're experienced.

My suggestion, beyond palacsint's, is that you slim your code down a bit to make it more readable. I cut 11 lines from get, decreasing it's length by 46%, without compromising on readability one bit.

public function get($start=1, $end=100) {
    $data = array();

    for($num = $start; $num <= $end; $num++) {
        foreach($this->fizzBuzz as $period => $text)
            if($num % $period === 0)
                $data[$num] = $text;
            else
                $data[$num] = $num;
    }

    return $data;
}

Of course this is personal preference, but I hope to inspire thought regardless of whether that would cause an actual change of mind or not.

PS. Sorry to comment on such an old question. I simply wanted to post my thoughts for anyone who might dig this up as I did.