Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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));
share|improve this question
1  
An important part of writing good code is choosing which code not to write. The above is an insanely over-engineered solution, and it would not hire somebody who produced it. I want you to do one days work per day. I do not want you to do one days work per month, which is how this code suggests you will work. I would be worried that you're the sort of person who produces a mini-framework to solve even the simplest of problems. –  meagar Jan 5 '13 at 16:59
1  
I have worked with people who write code this way. They are the worst people to work with. They move at a glacial pace and spend most of their time writing code that they will throw out when they realize they've produced a bloated, unmaintainable monster that is in desperate need of refactoring. The worst thing you can do to a codebase is introduce unneeded complexity. You're introducing tremendous space for maintenance headaches and potential bugs for no gain. –  meagar Jan 5 '13 at 17:12

2 Answers 2

up vote 8 down vote accepted

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:

share|improve this answer
1  
Thanks, they are good points. –  Paul Dec 19 '11 at 14:40

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.

share|improve this answer
1  
$data[$num] .= $text; will issue a warning if $data[$num] doesn't already exist. (In other words, this code is going to spit out a ton of warnings.) –  Corbin Jan 5 '13 at 4:45
    
Thanks Corbin, I didn't notice the dot. Fixed now! –  user20821 Jan 5 '13 at 13:48
    
I can not disagree more with your first line. This code does not speak of experience to me; it speaks of crippling inexperience. Only somebody who had never written code professionally could produce something so overwrought for such a simple task. I have worked with people like this. They take ten times longer than anybody else to solve the same problems. Given a half-day problem, they'll spend a full day writing most of a solution and then throw it out because they thought of a better architecture. –  meagar Jan 5 '13 at 17:06
    
The dot was there for a reason, because multiple periods may be present for a given number. –  Paul Jan 21 '13 at 12:34

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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