6
\$\begingroup\$

I've tried to write a configurable OOP-style FizzBuzz program in PHP for learning purposes. I hope everything is quite easy to understand.

  1. What do you think of the overall architecture?
  2. Are OOP principles kept well?
  3. What can be improved?
  4. How could error-handling and input-checking (i.e. currently you can break the program with ill-formed $multipliers array) be improved?

<?php

interface FizzBuzzable
{
    public function printMe();
}

class NumberFizzBuzzItem implements FizzBuzzable
{
    private $number;

    public function __construct($number)
    {
        $this->number = $number;
    }

    public function printMe()
    {
        return (string) $this->number;
    }
}

class WordFizzBuzzItem implements FizzBuzzable
{
    private $word;

    public function __construct($word)
    {
        $this->word = $word;
    }

    public static function check($number, $multiplier)
    {
        return !($number % $multiplier);
    }

    public function printMe()
    {
        return $this->word;
    }
}

class FizzBuzzPrinter
{
    private $items = array();

    public function add(FizzBuzzable $item)
    {
        $this->items[] = $item;
    }

    public function printMe()
    {
        $output = '';
        foreach ($this->items as $item) {
            $output .= $item->printMe();
        }

        return $output;
    }

    public function __toString()
    {
        return $this->printMe();
    }

    public function isEmpty()
    {
        return empty($this->items);
    }
}

class FizzBuzzFactory
{
    private static $multipliers = array();

    public static function init(array $multipliers)
    {
        self::$multipliers = $multipliers;
    }

    public static function create($number)
    {
        $printer = new FizzBuzzPrinter();

        foreach (self::$multipliers as $multiplier => $word) {
            if (WordFizzBuzzItem::check($number, $multiplier)) {
                $printer->add(new WordFizzBuzzItem($word));
            }
        }

        if ($printer->isEmpty()) {
            $printer->add(new NumberFizzBuzzItem($number));
        }

        return $printer;
    }
}

class FizzBuzzIterator implements Iterator
{
    private $from;
    private $to;
    private $number;

    public function __construct($from, $to, $multipliers)
    {
        $this->from = $from;
        $this->to = $to;
        FizzBuzzFactory::init($multipliers);
    }

    public function current()
    {
        return FizzBuzzFactory::create($this->number);
    }

    public function rewind()
    {
        $this->number = $this->from;
    }

    public function key()
    {
        return null;
    }

    public function next()
    {
        ++$this->number;
    }

    public function valid()
    {
        return $this->number <= $this->to;
    }
}

foreach (new FizzBuzzIterator(1, 100, array(3 => 'Fizz', 5 => 'Buzz')) as $fb) {
    echo $fb . "\n";
}

Update:

I modified the code according to @ChrisWue suggestions. The FizzBuzzPrinter class is gone as we don't actually need concatenation by the rules (this was premature optimization), check() method has been moved to FizzBuzzFactory which isn't static anymore. FizzBuzzable interface became FizzBuzzItem abstract class because a __toString() method implementation which was removed along with FizzBuzzPrinter is still needed for all concrete classes. Business logic was also changed a bit: krsort() is applied to $multipliers array in a factory constructor to ensure that bigger multipliers are processed first.

<?php

abstract class FizzBuzzItem
{
    abstract public function printMe();

    public function __toString()
    {
        return $this->printMe();
    }
}

class NumberFizzBuzzItem extends FizzBuzzItem
{
    private $number;

    public function __construct($number)
    {
        $this->number = $number;
    }

    public function printMe()
    {
        return (string) $this->number;
    }
}

class WordFizzBuzzItem extends FizzBuzzItem
{
    private $word;

    public function __construct($word)
    {
        $this->word = $word;
    }

    public function printMe()
    {
        return $this->word;
    }
}

class FizzBuzzFactory
{
    private $multipliers = array();

    public function __construct(array $multipliers)
    {
        $this->multipliers = $multipliers;
        krsort($this->multipliers);
    }

    public static function check($number, $multiplier)
    {
        return !($number % $multiplier);
    }

    public function create($number)
    {
        foreach ($this->multipliers as $multiplier => $word) {
            if (self::check($number, $multiplier)) {
                return new WordFizzBuzzItem($word);
            }
        }

        return new NumberFizzBuzzItem($number);
    }
}

class FizzBuzzIterator implements Iterator
{
    private $from;
    private $to;
    private $number;
    private $factory;

    public function __construct($from, $to, $multipliers)
    {
        $this->from = $from;
        $this->to = $to;
        $this->factory = new FizzBuzzFactory($multipliers);
    }

    public function current()
    {
        return $this->factory->create($this->number);
    }

    public function rewind()
    {
        $this->number = $this->from;
    }

    public function key()
    {
        return null;
    }

    public function next()
    {
        ++$this->number;
    }

    public function valid()
    {
        return $this->number <= $this->to;
    }
}

foreach (new FizzBuzzIterator(1, 100, array(3 => 'Fizz', 5 => 'Buzz', 15 => 'FizzBuzz')) as $fb) {
    echo $fb . "\n";
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Wow, this is even more over-engineered than the solution I proposed: Fizz Buzz interview question \$\endgroup\$ – Paul Nov 3 '13 at 4:15
  • 1
    \$\begingroup\$ Of course at first I had wrote this straight-forward one: codepad.org/hDSKRbv6, but then I wanted to try a configurable and extendable object-oriented solution instead of hard-coded procedural conditions. \$\endgroup\$ – pati Nov 3 '13 at 8:43
12
\$\begingroup\$

First of: I like the iterator approach. We use the FizzBuzz problem as part of our interview process and to this date nobody went down that route.

The main points I have:

  1. Apparently your solution is quite over-engineered for a problem like this so I just assume you did it for the lack of a proper example (to be honest I usually find it quite difficult to come up with simple, comprehensive examples where the application of more complex designs doesn't seem over-engineered so fair enough :)).
  2. Your FizzBuzzFactory should not be static. This for example prevents running multiple FizzBuzz games simultaneously. Just create an instance with the specific multipliers as a member and store it as an additional member of your iterator. Static classes which hold states are evil (there are probably sensible applications for it but in general terms they are bad) as they create problems with unit testing, parallelization, re-use, modularity, etc.
  3. You have hidden the actual business logic in the WordFizzBuzzItem class as a static method. A more central place to have that function would be the FizzBuzzFactory as it is the one which effectively decides which FizzBuzzable to create.
  4. The most common definition of the FizzBuzz game I know of is this:

    Write a program that prints the integers 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"

    However you have implemented this:

    Write a program that prints the integers 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 the concatenation of both.

    Effectively this means you only have to write array(3 => 'Fizz', 5 => 'Buzz') as rules instead of array(3 => 'Fizz', 5 => 'Buzz', 15 => 'FizzBuzz') so you saved yourself some typing work by reducing a case to a combination of the existing ones.

    Why does it matter? If you consider the problem as the business logic given provided by a customer then approx. 5sec after the deployment of your solution the customer will come back and say: "Ah yes, I forgot, if it's divisible by 3 and 5 you have to print FixBugz because some of our legacy applications which we can't change have a typo in their parsing code." Now instead of just changing array(3 => 'Fizz', 5 => 'Buzz', 15 => 'FizzBuzz') into array(3 => 'Fizz', 5 => 'Buzz', 15 => 'FixBugz') you have to change a whole bunch of implementation code and unit tests.

    In the end you have generalized a solution which might not fit the requirements because the requirements have changed (they always do).

\$\endgroup\$
  • \$\begingroup\$ Thanks a lot! I thought about some of your points (3, 4) but was unsure. Now if I implement these suggestions and simplify the 15 case, there's no longer a need for FizzBuzzPrinter which was effectively a wrapper for concatenation, the factory can return a FizzBuzzable instance directly. Hence this part was indeed over-engineered and prematurely assumed about the logic. Your point 2 is also very valuable, I usually see factories as static in examples over the web so I perhaps blindly copied that approach, a factory instance can be really beneficial here for modularity. \$\endgroup\$ – pati Nov 3 '13 at 8:41
  • 2
    \$\begingroup\$ I was first introduced to FizzBuzz (or rather, an game isomorphic to it, called "Buzz") in 1978. The game has many variations, but all the ones I know of follow the principle that when multiple patterns are matched, you concatenate the words associated with those patterns. The "most common" definition above is not necessarily the best definition. If you want that definition to be taken literally, not parsed as "concatenate the words as in other variations of this game", I would recommend that the initial requirements map multiples of 15 to some word other than "FizzBuzz". \$\endgroup\$ – David K Jul 12 '14 at 21:49

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.