Does this break open closed principle

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'));
    }

}

Answer 2

The open-closed principle is not universal. You need to make an assumption about what is likely to change (the open part) and what is not (the closed part).

Since you're using a factory, the closed part is the create service (factories make this part closed). The open part is the things the factory is going to create. A factory allows extending those things later.

A small but important point is that your pattern is not a GoF factory, but rather a Simple Factory. So, it's perhaps not the strongest form of Factory for exploiting the open-closed principle. That is, if you add new stuff to create, you have to modify the class (the $codeMap array).

What stands out in your question is that you seem to contradict the principle of openness when you say:

The list of codes are a member of the class, since they should never change.

In my mind, if you're using a factory, the list of codes is expected to change.

As for your set function, it's a public method, and so by definition is closed (else you shouldn't reveal it). On the other hand, you're exposing details of the implementation (as mentioned in Crackertastic's answer). You might be concerned more about violating encapsulation with this method.

I think an easier solution (although I'm not sure about it in PHP) is to initialize your factory with a $codeArray that's been created by another class. I think this is what Kamal Wickamanayake refers to in his comment on your question. Another solution is a service (closed) to add/delete elements (which boil down to adding new entries into your $codeArray but in a hidden way).