这是否打破了封闭原则

Question

I am attempting to create a factory. I want the client to send a code to the create method, which will be used to instantiate a class that is used to process that type of 'thing'.

The list of codes are a member of the class, since they should never change. But, to make it more testable i have added a setter for the codeMap array.

Does this break the open closed principle and if so, how to make this testable correctly?

<?php

class My_ThingFactory
{
    /**
     * @var array
     */
    private $codeMap = array(
        'A111' => 'My_Thing_ConcreteA'
    );

    public function create($code)
    {
        if (isset($this->codeMap[$code])) {
            return new $this->codeMap[$code];
        }
    }

    public function setCodeMap(array $codeMap)
    {
        $this->codeMap = $codeMap;
    }
}

Answer 1

The Open/Closed principle has to do with extending some code to add functionality without modifying the core behavior (i.e. by not editing the source code) of your class. Your class keeps its internals to itself and provides clear public interfaces to interact with them. From this perspective, no you have not broken open/closed principle. At least not at face value.

However, that said, I also got the impression from your question that you are wondering if having a setter for your private $codeMap array breaks the principle. It doesn't directly, but the implementation also makes it attractive to modify if another developer wants more fine tuned access to the $codeMap array. Basically, the only way to update this array on the fly is to wipe it out and reset it with setCodeMap(). You are not providing a mechanism to add a single code to the map. As soon as you find yourself needing more granular access to this map, you will also find yourself violating the open/closed principle.

Consider this, let's say another developer is using your code and the $codeMap array is 20 or 30 elements strong; they must hack your core code to provide better access to that array. Since there is not way to add a single code, they must create a new array to pass to setCodeMap() that consists of the current $codeMap array plus any additional elements they wish to add. There isn't another way (besides hardcoding the original array) to do this without opening up the My_ThingFactory and adding something like:

public function getCodeMap()
{
    return $this->codeMap;
}

Then in their extended class they could do something like:

class AnotherThingFactory extends My_ThingFactory
{    
    public function addCodes(array $newCodes)
    {
        $this->setCodeMap(array_merge($this->getCodeMap(), $newCodes));
    }    
}

But again, this is only possible by going into your class and adding the needed functionality before, which does break the open/closed principle. You could also rectify this by simply making the $codeMap property protected and then an extending class can do what they need to without hacking your code. The extending class also then has the onus of ensuring that they are manipulating it correctly.

So to answer the open/closed question: if you are intending to keep the $codeMap locked down by design and don't intend for it to be used in alternate way then you are fine. But as I said above as soon as you need better control of the $codeMap array, you will need to violate the principle to do so. My suggestion would be to brainstorm how much management of that factory you want built in to your class and make it part of the class core functionality.

As for testing, I don't see why you couldn't test this code as it is. You can set your code map and then test for the corresponding instance that was returned with the create() method.

class FactoryTest extends PHPUnit_Framework_TestCase
{

    private $factory;

    public function setUp()
    {
        $this->factory = new My_ThingFactory();
    }

    public function tearDown()
    {
        $this->factory = null;
    }

    public function testMadeConcreteA()
    {
        $this->assertInstanceOf('My_Thing_ConcreteA', $this->factory->create('A111'));
    }

    public function testMadeStealthBomber()
    {
        $this->factory->setCodeMap(array('B-52', 'StealthBomber'));  //Assume the class exists.
        $this->assertInstanceOf('StealthBomber', $this->factory->create('B-52'));
    }

    public function testDidntMakeSquat()
    {
        $this->assertNotInstanceOf('My_Thing_ConcreteA', $this->factory->create('Nada'));
    }

}