Why you should not access private fields, even if you can

TL;DR;
The only method accessing private (or protected) field should be it’s accessor. This accessor can has any visibility. It will save some headache for you later.

A quick example:

Assume, you’ve inherited code. Your Product model looks like this:


    private $_options

    public function getOptions()
    {
        return $this->_options;
    }

    public function getOption($code)
    {
        if (isset($this->_options[$code])) {
            return $this->_options[$code];
        }
        return null;
    }

    public function setOptions(array $options)
    {
        $this->_options = $options;
    }

As you can see – author thought that this is a good idea that you set options before you use it later in the code.

But the time came, and you have to alter that behavior. Options are needed even if they were not set before (i.e. fetched from database), so you override getOptions:

    public function getCustomOptions()
    {
        if(null == $this->_customOptions) {
            $options = $this->fairlyComplicatedWayToFetchOptions()
            $this->setOptions($options);
        }
        return parent::getOptions();
    }

Looks good. But because author was accessing private fields whenever he felt like doing so – you have more work to do. You also have to fix getOption($code) as well.

    public function getCustomOption($code)
    {
        return $this->getCustomOptions()[$code]?: null;
    }

I think it would be a good rule for PHP code that there’s one and only one method which access private fields directly. Even if rules of OOP allows you to access private field – it’s most definitely bad idea to do so. If you need no public getter – make private one.

It would be a simple way to keep “hygene” of your code. Do you agree?

Share Button

How to simplify test with right pattern

Recently I stumbled upon some tests which were quite messy, and looked more or less like this:

Tested method:

   
    public function foo($mailer, $selectedPaymentMethod)
    {
        switch($selectedPaymentMethod) {
            case 0: {
                $mailer->caseOneMethod();
                break;
            }
            case 1: {
                $mailer->caseTwoMethod();
            }
        }`
    }

It’s of course simplified, but you’ll find similar stuff in your code if it’s old enough and you look closely.

The test method was mock mess:

    
public function test_foo_calls_CaseOneMethod_when_state_is_zero()
    {
        $object = new SwitchCase();
        $serviceStub = $this->getMock("stdClass", ['caseOneMethod', 'caseTwoMethod']);

        $serviceStub->expects($this->once())
            ->method('caseOneMethod');

        $object->foo($serviceStub, 0);
    }

This is only one case. To have it fully covered I should create method for state 1. And the add requirements that the other method is not called at all. Lot’s of work, and more coming if more cases appear (remember – this is payment method, and those are never enough for your client).

This is road to nowhere. If such simplified example seems not to be problematic – try for yourself add 5 cases. Then manage in some reasonable way list of methods which should and should not be called. How do you refactor? Extract $methodsWhichShouldNotBeCalled, use dataProviders? But there’s an easier way.

Enter polymorphism. It’s not just something you had to learn on your OOP course and what sophisticated programmers talk about. It’s also making programming simpler. And since writing test is also programming – it will make your tests simpler.

First I defined interface, and simple workers which represent different methods which has to be performed for different payment methods:

interface IMethods
{
    public function doYourJob();
}

class MethodOne
    implements IMethods
{
    public function doYourJob()
    {
        return __CLASS__ . " doing it's job";
    }
}

class MethodTwo
    implements IMethods
{

    public function doYourJob()
    {
        return __CLASS__ . " doing it's job";
    }

}

Now I move decision tree to new class which job is to return method depending on conditions:

 
class MethodFinder
{

    /**
     * @param int $selectedPaymentMethod
     * @return IMethods
     */
    public function findMethod($selectedPaymentMethod)
    {
        switch($selectedPaymentMethod) {
            case 0: {
                return new MethodOne();
            }
            case 1: {
                return new MethodTwo();
            }
        }
    }

}

You can think: nothing changed, I still have this damn switch case. But look at the test:

class MethodFinderTest
    extends \PHPUnit_Framework_TestCase
{

    /**
     * @dataProvider statesToMethodsMapping
     */
    public function test_methodFinder_finds_correct_method($paymentMethod, $expectedClass)
    {
        $methodFinder = new MethodFinder();

        $this->assertInstanceOf($expectedClass, $methodFinder->findMethod($paymentMethod));

    }

    public function statesToMethodsMapping()
    {
        return [
            [0, 'MethodOne'],
            [1, 'MethodTwo'],
        ];
    }
}

The difference is immense!

Now the test is about what method returns, not what is and what isn’t called internally. No mocking here, and simple array is easy to read – which paymentMethod means which method.

Original method also changes a lot:

class PolyExample
{

    public function foo($paymentMethod)
    {
        $methodFinder = new MethodFinder();
        $method = $methodFinder->findMethod($paymentMethod);
        $method->doYourJob();
    }

}

No branching. Of course – first line also needs some attention (it’s a implicit dependency of your class, so you would rather pass methodFinder as a param or use some kind of service locator to make it mockable and be able to test that “doJourJob” is called on whatever it returns. But that’s a detail and it depends on your architecture and other constraints.

As a closure I’ll tell you a secret: it’s a design pattern. I honestly don’t remember how it was exactly called. Some mixture of strategy and factory… I guess. To find out read about patterns and try to find the one that fits ;) This is your homework – leave a comment when you find out.

Also you may play with this code by yourself: https://github.com/pawlik/ScratchPad/tree/SwitchCaseVsFactory

Share Button

Github surprise

Three months ago I asked GH if it’s somehow possible to search pull requests by branch. It was not possible at that time.

It’s a nice surprise that they eventually implemented it… but it’s even more surprising that they e-mailed me about that! (after three months!)

You can now search for pull requests by branch name.

`is:open base:staging` will find all open pull requests that will be merged into a “staging” branch (https://github.com/search?q=is:open+base:staging)

`is:open head:feature` will find all open pull requests that want to merge code from a “feature” branch (https://github.com/search?q=is%3Aopen+head%3Afeature)

The search uses prefix matching so the entire branch name does not have to be typed into the search field. The search for `is:open head:feature` will also match a branch named “feature-api-enhancement”.

We hope you enjoy this new feature. As always, please send us an email if you have any questions.

TwP

I wonder what kind of system they use – my support request/ticket must have been linked with feature/user story.

Neat.

Share Button