Refactoring procedural and semi-OOP code

[andrewjbaker]

Hopefully this post will be useful to those of you looking to migrate your codebase to all OOP; Chris, please ignore. 

Here's what I had originally:

Code: [Select]

class MapReference
{
    // ...
}

define('EASTING_INCREMENT',  5000);
define('NORTHING_INCREMENT', 5);

$Map = array(
      530610 => new MapReference(530610, '1A')
    , 535610 => new MapReference(535610, '1B')
    // ...

    , 530605 => new MapReference(530605, '2A')
    , 535605 => new MapReference(535605, '2B')
    // ...
);

function referenceExists($reference)
{
    global $Map;

    if (!is_int($reference)) {
        throw new Exception();
    }

    // ...

    return isset($Map[$reference]);
}

// ...

class Character
{
    // ...

    protected $_reference;

    // ...

    public function getReference()
    {
        return $this->_reference;
    }

    public function setReference($value)
    {
        // ...

        $this->_reference = $value;
    }
}

class PlayerCharacter extends Character
{
    // ...

    public function goNorth()
    {
        if (referenceExists($this->getReference() + NORTHING_INCREMENT)) {
            $this->setReference($this->getReference() + NORTHING_INCREMENT);
        }
    }

    // ...
}

Of course, all of the classes were in their own PHP file and I've omitted some of the code where you see the // ... comments. In short, the code has recently been refactored using the Singleton design pattern to the following:

Code: [Select]

class MapReference
{
    // ...
}

final class Map
{
    const EASTING_INCREMENT  = 5000
        , NORTHING_INCREMENT = 5;

    protected static $_instance;

    protected static $_map;

    private function __construct()
    {
        self::$_map = array(
              530610 => new MapReference(530610, '1A')
            , 535610 => new MapReference(535610, '1B')
            // ...

            , 530605 => new MapReference(530605, '2A')
            , 535605 => new MapReference(535605, '2B')
            // ...
        );
    }

    private function __clone() { }

    public static function getInstance()
    {
        if (self::$_instance === NULL) {
            self::$_instance = new self();
        }
        return self::$_instance;
    }

    public function referenceExists($reference)
    {
        if (!is_int($reference)) {
            throw new Exception();
        }

        // ...

        return isset(self::$_map[$reference]);
    }

    // ...
}

class Character
{
    // ...

    protected $_reference;

    // ...

    public function getReference()
    {
        return $this->_reference;
    }

    public function setReference($value)
    {
        // ...

        $this->_reference = $value;
    }
}

class PlayerCharacter extends Character
{
    // ...

    public function goNorth()
    {
        $map = Map::getInstance();

        if ($map->referenceExists($this->getReference() +
            Map::NORTHING_INCREMENT)) {
            $this->setReference($this->getReference() +
                Map::NORTHING_INCREMENT);
        }
    }

    // ...
}

Note that, irrespective of the code used, the approach adopted in the calling code hasn't changed in any way at all. It is still...

Code: [Select]

$playerCharacter = new PlayerCharacter();
$playerCharacter->setReference(535605);
echo $playerCharacter->getReference() . PHP_EOL;
$playerCharacter->goNorth();
echo $playerCharacter->getReference() . PHP_EOL;

Any comments gratefully welcomed. 

Cheers, Andy.

[JGadrow]

Why did you choose to make the PlayerCharacter class a Singleton object? Singletons are most useful when working with "handlers" rather than actual data instances. Factories, Custom Error Handler, etc. are great to use a Singleton pattern on.

Also, if you do end up needing multiple Singleton objects, using PHP 5.3 you can actually make a base Singleton object and have the other objects extend that base object so you don't need to write your Singleton methods repeatedly.

[andrewjbaker]

I haven't made the PlayerCharacter class a Singleton. The only Singleton class in the code I've posted is Map. 

I'm still using 5.2.x at the moment but I'll bear the base Singleton object shizzle in mind if I find myself in a position to upgrade to 5.3. 

[Harkins]

I want to see Singleton renamed to Globalton.

[Chris]

Chris, please ignore. 

Seems you came to understanding of our community quite fast

[JGadrow]

I haven't made the PlayerCharacter class a Singleton. The only Singleton class in the code I've posted is Map.

D'oh! That's what I get for only skimming the code! lol

I'm still using 5.2.x at the moment but I'll bear the base Singleton object shizzle in mind if I find myself in a position to upgrade to 5.3. 

I try to work in 5.3 whenever possible. It has even greater support for OOP (thank you for giving us namespaces finally!) and the fact that it's a shift towards how things will work in PHP6. Now that Drupal is supposed to be 5.3 compatible (still haven't tested this) I'm going to test it out and see if I can't convince my clients to make the switch. There are reports that state 5.3 has improved performance levels quite considerably, especially in the database connectivity level!

@Harkins: I don't think Globalton would catch on. But Global Class might!

[Harkins]

@Harkins: I don't think Globalton would catch on. But Global Class might!

I've never understood why people bother with all that mucking around with instances. Just tag the object initialization on to the end some startup script like the database initialization and add 'global $map;' anytime you want to access it. It's a global variable, most languages have very succinct idioms for managing those, unfortunately.

[andrewjbaker]

My justification for switching from a global variable to a Singleton for my world Map data structure is twofold:

1. I'm trying to ensure that the codebase for my RPG is as OO as possible because I know that OOA&D leads to easier to understand (at a higher level of abstraction), more flexible and extensible code.
2. I plan on migrating the statically stored map to a dynamic, DB equivalent if there is a justifiable performance improvement to be had; creating upwards of 1,000 instances of MapReference might well become unworkable where actually only a handful of tiles actually need to be instantiated per page view.

As the codebase is now, all I'd need to do is replace the referenceExists method (in the Map class) with something akin to:

Code: [Select]

    public function referenceExists($reference)
    {
        if (!is_int($reference)) {
            throw new Exception();
        }

        // ...

        $link = Mysql_connect();
        $fmt = <<<EOF
SELECT *
FROM map_reference
WHERE reference = %d
EOF;
        $query = sprintf($fmt, $reference);
        if ( ($result = mysql_query($query, $link)) === FALSE) {
            throw new Exception();
        }
        $exists = (mysql_result($result, 0) == 1);
        Mysql_close($link);
        return $exists;
    }

Chances are good, I guess, that my wrapper Mysql_xxx() functions would also be replaced with a Singleton. 

None of the code outside of the Map class will need altering, thus simplifying support and maintenance.

Yes, sometimes OOP can be overkill... and at others it eases development further down the line. 

[Harkins]

My justification for switching from a global variable to a Singleton for my world Map data structure is twofold:

I don't see a significant difference between the two.

[andrewjbaker]

With a Singleton the actual implementation of map is hidden from the calling code. The code that uses the map doesn't have to worry whether the underlying data structure used to represent the map is an array, a dictionary, a linked list, or whatever.

Encapsulating my map means that I am free to alter it without impacting on my calling code; this becomes very important when your data structures are referenced in a whole bunch of places within your codebase.

[Harkins]

That's not different from a global variable holding an object.

[andrewjbaker]

I've selected the Singleton for the two reasons I've previously stated. I hadn't intended to get involved in a lengthy debate over global variables versus Singletons; I'm sure others have debated this exact same issue before and that a quick Google should help to clear up any pros and cons for those that are curious.

In the meantime, I'll enjoy the ordered structure a Singleton has brought to the design of my object model and the ease of implementing lazy instantiation it now affords me.