1
\$\begingroup\$

A previous developer had an array of strings like so:

public static const PAGE_ONE:String     = 'PageOne';
public static const PAGE_TWO:String     = 'PageTwo';
public static const PAGE_THREE:String   = 'PageThree';
public static const PAGE_FOUR:String    = 'PageFour';
public static const PAGE_FIVE:String    = 'PageFive';
public static const PAGE_SIX:String     = 'PageSix';

When he wanted to open a new type of page he would write

switch ($page)
{
    case PAGE_ONE:
        // Do stuff
        break;

    ...

    case PAGE_SIX:
        // Do stuff
        break;
}

Which was all well and good, but I thought it could be condensed a little bit, so I created a little more functional approach.

In the constructor, this function is called setting up the table:

    private function setDefaultPages() : void {
        _pages[PAGE_ONE]          = function($xml:XMLList)  { return new PageOne( $xml ); }

        ...

        _pages[PAGE_SIX] = function($xml:XMLList)   { return new PageSix( $xml ); }
    }

Now, when the time comes to open a page, all I have to write is this:

_nextPage = _pages[$page](xml);

However, I am worried that I have hurt readability by doing this. Could anybody suggest a good middle-ground if it is so?

\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

There's a safety issue, here. If you somehow get a bad page name in the original case statement, nothing good happens, but nothing bad happens either. What happens for _nextPage["BOGUS"]($xml); ?

That aside, your change can be broken down into a few steps and each one evaluated separately. Also, the solution has parts that can be teased out for readability.

You seem to be implying that

switch ($page)
{
    case PAGE_ONE:
        // Do stuff
        break;
    // etc.

can be replaced by:

switch ($page)
{
    case PAGE_ONE:
        _nextPage = new PageOne($xml);
        break;
    // etc.

That being the case, you could stop there.

Optionally you could wrap the switch statement in a function:

private function generateNamedPage($page:String, $xml:XMLList) {
    switch ($page)
    {
        case PAGE_ONE:
            return new PageOne($xml);
            break;
    // etc.

_nextPage = generateNamedPage($page, $xml);

It's no harder to extend this code than to extend your setDefaultPages code. And it's easier to follow.

Dispatching like this to create new objects of different kinds is a common technique known as the Abstract Factory Pattern. Naming and comments that reference this pattern will help to clarify your intent to anyone familiar with the pattern.

The only benefit I can see to the more sweeping change using setDefaultPages is to enable dynamic reconfiguration, e.g. from anywhere else in the code after setDefaultPages has run:

    if (userHasAdminPrivileges()) {
        // add admin navigation to front page
        _pages[PAGE_ONE] = function($xml:XMLList)   { return new PageOneForAdmins( $xml );}

        // add access to admin pages
        _pages[PAGE_SEVEN] = function($xml:XMLList)   { return new PageSeven( $xml ); }
        _pages[PAGE_EIGHT] = function($xml:XMLList)   { return new PageEight( $xml ); }
    }

If you don't need this kind of dynamism, keep it simple with the switch statement.

On the other hand, if the more dynamic original solution is desired, its parts can be packaged to improve readability, much like the wrapping of the switch statement above. Caveat: I don't know as3, but if I did, I might declare specific return types for these functions.

// return a function
// This may be overkill if it's only ever used by generateNamedPage.
private function getNamedPageGenerator($page:String) {
    // probably want to guard against bogus $page, returning a dummy page generator function
    return _pages[$page];
}

// return a new page
private function generateNamedPage($page:String, $xml:XMLList) {
    return getNamedPageGenerator($page)($xml);
    // OR, to be more explicit:
    // var generatePage = getNamedPageGenerator($page);
    // return generatePage($xml);
}
...
_nextPage = generateNamedPage($page, $xml);

The name _pages probably adds to the confusion because it is not a collection of pages at all, but a collection of page-generator functions. Maybe calling it _pageGenerator or _pageFactory would be better.

\$\endgroup\$
1
  • \$\begingroup\$ Some excellent tipps, I didn't realize that this pattern had a name :D Yes, I actually do dynamically change the pages like you are describing, which was one of the catalysts for the change. I thought it would be a little easier to extend than the previous version, although I suppose palacsint would be the end-all overkill solution to this problem. \$\endgroup\$
    – IAE
    Jan 20, 2012 at 17:29
2
\$\begingroup\$

It seems to me that what you need is the State pattern.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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