Removing repeated conditions using polymorphism and the Factory pattern

Question

<?php
/**
 * My codebase is littered with the same conditionals over and over
 * again.  I'm trying to refactor using inheritance and the Factory
 * pattern and I've had some success but I'm now stuck.
 *
 * I'm stuck because I want to derive a new class from the one
 * returned by the Factory.  But I can't do that, so I'm obviously
 * doing something wrong somewhere else.
 */


/**
 * The old implementation was as follows.  There's if statements
 * everywhere throughout both LayoutView and ItemIndexView and
 * SomeOtherView.
 */
class LayoutView { }
class IndexView extends LayoutView { }
class SomeOtherView extends LayoutView { }

/**
 * Below is the new implementation.  So far I've managed to tidy
 * up LayoutView (I think I have anyway).  But now I'm stuck because
 * the way I've tidied it up has left me not knowing how to extend
 * it.
 *
 * For example's sake, let's say the conditions were relating to a
 * type of fish: salmon or tuna.
 */
abstract class LayoutView {
    protected function prepareHeader() { echo __METHOD__, "\n"; }
    protected function prepareLeftHandSide() { echo __METHOD__, "\n"; }
    protected function prepareFooter() { echo __METHOD__, "\n"; }
    public function prepare() {
        $this->prepareHeader();
        $this->prepareLeftHandSide();
        $this->prepareFooter();
    }
}

class SalmonLayoutView extends LayoutView
{
    protected function prepareLeftHandSide() { echo __METHOD__, "\n"; }
}

class TunaLayoutView extends LayoutView
{
    protected function prepareLeftHandSide() { echo __METHOD__, "\n"; }
    protected function prepareFooter() { echo __METHOD__, "\n"; }
}

class ViewFactory {
    public static function getLayoutView($fishType) {
        switch($this->$fishType) {
        case 'salmon':
            return new SalmonLayoutView();
            break;
        case 'tuna':
            return new TunaLayoutView();
            break;
        }
    }
}

/**
 * Now LayoutView has been cleaned up and the condition that was once
 * scattered through every LayoutView method is now in one place.
 */
$view = ViewFactory::getLayoutView( Config::getOption('fishtype') );
$view->prepare();

/**
 * Now here's where I'm stuck.  I want to effectively extend whatever
 * class $view is an instance of.
 *
 * The reason being, I wish to derive a view to show an index of
 * articles within the appropriate *LayoutView.  The IndexView code is
 * the same for Salmon and Tuna.
 *
 * I can do something like this:
 */

class SalmonIndexView extends SalmonLayoutView { }
class TunaIndexView extends TunaLayoutView { }

/**
 * But then I'll be writing the same IndexView code twice.  What I'd
 * like to do is something like this:
 */

$view = ViewFactory::getLayoutView( Config::getOption('fishtype') );
class IndexView extends get_class($view) { }

/**
 * But I'm pretty certain that's not possible, and even if it was
 * it seems very wrong.
 *
 * Can someone more experienced in refactoring and OO please let
 * me know where I've gone wrong and suggest a clean way to solve
 * this?
 */

Answer 1

If the IndexView code really is the same then you don't need inheritance, but composition. Add, in your base LayoutView class, an instance of IndexView that then you'll be able to call from each *LayoutView.

Inheritance is due only when the relationship between objects is is-a. I deduce that an IndexView is not a LayoutView, but rather the LayoutView has an IndexView.

Check this out, I don't agree with everything it says, but still: http://phpimpact.wordpress.com/2008/09/04/favour-object-composition-over-class-inheritance/

Answer 2

Thanks for your answer, it makes sense. The IndexView shouldn't be derived from LayoutView. Instead, the LayoutView should allow for an arbitrary subview?

I can implement this in the context of the example I posted, but in my real application I'm having a lot of trouble. I think it's due to the way the framework (WACT 0.2a) works. While it supports composable templates, it doesn't support view composition.

The WACT View class has exactly one Template object. The templates are composable in that you can include other template files and also "wrap" a template file inside another at a given placeholder. All this is done through custom XML tags in the template files themselves. Example:

layout.html:

<html>
<body>
<div id="content">
  <core:placeholder id="content" />
</div>
</body>
</html>

inner.html:

<core:wrap file="layout.html" placeholder="content" />
<!-- this line and below is inserted inside <div id="content"> -->
<!-- etc.. -->

The View classes for this look like so:

class LayoutView extends View {
    public function prepare() {
        parent::prepare();
        /* full of dirty if-statements that I'm trying to factor away */
        if (FISHTYPE == 'salmon') { /**/ }
        if (FISHTYPE == 'tuna') { /**/ }
    }
}

class IndexView extends LayoutView {
    /**
     * no dirty if's in here, this part of the view is the same
     * for both salmon and tuna.  although there are other derived
     * views (from LayoutView) that do have ifs on FISHTYPE.
     */
    public function prepare() {
        parent::prepare();
    }
}

When a view is displayed, it first calls ->prepare() to export the data to the template. The template is a member variable of the View class. This is why I'm stuck. I can't have IndexView and LayoutView operating on the same Template without exposing the Template member variable. Or can I?

Below is a simplified example of the framework's View and Template classes, showing the above paragraph in actual code.

// A simplified example of WACT's View class
class View {
    protected Template $template;
    public function __construct($templateFilename) {
        $this->Template = new Template($templateFilename);
    }
    public function prepare() {
        // empty, to be filled out by derived classes
    }
    public function display() {
        $this->prepare();
        $this->Template->render();
    }
}

// A vastly simplified example of WACT's Template class
class Template {
    protected $data;
    public function __construct($templateFilename) {
        // open template file
    }
    public function set($key, $value) {
        $this->data[$key] = $value;
    }
    public function render() {
        // render the file with $this->data in it
    }
}

Can you see the problem I'm facing?

It seems because of the nature of this <core:wrap> tag, only a single View object should be related with a single Template object (and therefore template file). Which means View must be built up by inheritance?

I agree with you I should make View composable (that is what you're saying right?) but I'm not sure how to do that here. Am I missing something? Or can you offer any advice on how to make the best of this situation?

Answer 3

Just pass the template as a parameter to the subviews you'll compose. I don't think that'd be evil in this case. Although if it's a standard framework you might be better asking in their forums, because they might have a functionality we are unaware of for this case (it usually happens)

You could have something like

class LayoutView {
    protected View $subview; //Potentially an array of views
    public function prepare() {
        // empty, to be filled out by derived classes
    }
    public function setSubView($view) { $this->subview = $view; }
    public function display() {
        $this->prepare();
        $this->subview->prepare($this->template);
        $this->template->render();
    }
}

class IndexView {
    protected View $subview; //Potentially an array of views
    public function prepare() {
        // empty, to be filled out by derived classes
    }
    public function prepare($template) {
       //operate on it, maybe even assigning it to $this->template
    }
    public function setSubView($view) { $this->subview = $view; }
    public function display() {
        $this->prepare();
        $this->subview->prepare($this->template);
        $this->template->render();
    }
}