In Further Defense Of Avoiding Private Methods

This is a rebuttal post to comments posted Private Methods Considered Harmful

I do not wholeheartedly believe that private methods are evil, or that they were mistakenly included in the PHP language by the core development team. Nor do I believe that there are only two true options when it comes to devising visibility requirements: public and protected. There is a place for private methods, in PHP development and elsewhere.

However, this does not offer developers a carte blanche to use private methods every which way they so choose. For code that is meant to be or needs to be extended, a private method is an inadequate and impossible solution.

Take the following for example:

<?php

class TestObject {
    
    private $_string;
    
    public function __construct($string = null) {
        if($string) {
            $this->setNewString($string);
        }
    }
    
    public function printString() {
        print $this->_string;
    }
    
    public function setNewString($string) {
        $this->_setString($string);
    }
    
    private function _setString($string) {
        $this->_string = $string;
    }
    
}

class ExtendedTestObject extends TestObject {
    
    public function setUppercaseString($string) {
        if ($string) {
            $this->_setString(strtoupper($string));
        }
    }
}

This code is extremely simple and intended to illustrate a specific point. The ExtendedTestObject class cannot override or access the private _setString() method. It does have the ability to access all the public methods, including setNewString(), though there may be good reasons for avoiding setNewString() for this particular implementation. However, the developer of ExtendedTestObject does not have the option to avoid using setNewString() if they want to set a string, and anything that setNewString() does that is unpleasant also cannot be avoided (the fact that at present it does nothing undesirable is not the point; the code is purposefully simple for illustration).

Though you might say that the developer can simply override any unpleasant behavior in setNewString() by extending it and redefining it, consider that the developer cannot simply redefine it if they wish to continue using the internal API of TestObject, since all the internal methods are marked private. They would instead be required to totally reimplement the methods that setNewString() calls that are private, and with complex code this can be extremely difficult.

Finally, no subclass can make direct access of the private property $_string. Even if we were to subclass it and want to override the printString() method, we would be unable to do so because we wouldn’t be able to access the private $_string property.

This may well be what the developer intended. When deriving an algorithm for example, this may be the intended behavior and developers may not wish to have the algorithm overridden in child classes. But for the vast majority of other methods, this simply doesn’t make sense. If a developer truly wished to prevent extension of the _setString() method, they could have declared it protected final; this would have allowed access in subclasses and also allowed some extension of the class by future developers, but prohibited redefinition of the _setString() method. Everybody wins.

I am not against the use of private methods, when they are used correctly. I am against their overuse, or their use to protect things that needn’t be protected. Employing the “take my ball and go home” methodology of programming is counterproductive to future users of one’s code. If the purpose of code is extension, as it so often is with libraries and frameworks, overzealous use of the private visibility keyword only serves to make the code harder to use, not easier.

Brandon Savage is the author of Mastering Object Oriented PHP and Practical Design Patterns in PHP

Bradley Holt (@BradleyHolt) wrote at 12/11/2012 12:52 pm:

This is a much more balanced post than your original “Private Methods Considered Harmful” post. In that post you took an absolutist position. The title alone implied that there were no good uses of private methods, ever. You also said, “Many developers believe that the internal methods of their objects should be private. While this thought process is understandable, it’s also wrong.” Thank you for this more reasoned update.

I think one point of confusion for folks is the concept of extensibility. This confusion, I believe, stems from the use of the keyword “extends” to indicated that one class inherits from another. I think people mistakenly conclude from this that inheritance is the only form of extension. Others who know better might mistakenly conclude that inheritance is the preferred approach to make code extensible.

In fact, one should favor composition over inheritance. Inheritance is one way to provide extensibility, but I would argue it is actually the least preferable approach. There are many design patterns that you can use to provide extensibility without resorting to simply using inheritance.

In my comments on your previous post, I mentioned the strategy pattern and the template method pattern. Those are two examples of design patterns that will allow you to provide extensibility without just naively using inheritance.

Sven Rautenberg (@SvenRtbg) wrote at 12/11/2012 2:57 pm:

I look at your example, and I see good design. The TestObject class actually does it all right. Let’s assume for a moment that besides offering to set and get the internal string, it MIGHT actually do something with it. A string object class, just like ArrayObject.

Guess what: The authors of ArrayObject intentionally did not expose anything of the inner workings of this object. All you get is the API consisting of only public methods, and you have to deal with it. It does not prevent you from extending it, though. So you are able to actually create a better version of all the public methods, and you can add functionality, but you cannot interfere with the inner workings.

Back to your TestObject. Making “_setString” protected does not help at all, because you also have to make the private property protected.

In fact, the right way to deal with your class is to use the API that is intended for this. Use parent::setNewString() to actually set the modified string. Or go another route and modify getString(). It is actually a good thing to do this, it is called decorator pattern.

But let’s assume for a moment that both classes are owned by a single entity. This voids the “cannot fix the bug otherwise” argument from your other posting. You can do everything you want to your classes.

But what if you do? You might want to refactor TestObject completely, all internal workings will be changed. If you exposed internals via protected methods and properties, you also have to check, more likely also change all classes that extend TestObject, because they might stop working.

A protected method is a STOP-sign: “No changes allowed here, because otherwise you will break things in other classes.” A protected method is part of the public API of this class, as I said.

Now if you write a library that anyone is intended to use, and you have no control over any extending class, the more protected methods and properties you offer, the less you can change anything in the library. Protected methods disable your ability to change things.

But of course they ENABLE the users of the class to change the class through inheritance. The question is: Is this the only pattern that can be used to change classes? I believe “no”.

Heikki Naski wrote at 12/16/2012 6:22 am:

Some cons of overusing protected visibility are reduced information hiding, increased scope of attributes, accusing visibility for bad design of class, adding dependencies within the class and adding to the public API.

———

“However, the developer of ExtendedTestObject does not have the option to avoid using setNewString() if they want to set a string”

This is exactly one of the most important benefits of making attributes private. When we hide the information where and how the data is obtained from, we also create single point of control where we can assert that all access to the data is uniform and constrained in similar ways. Think invariants.

Setting attributes private also reduces the scope of them to only that class, which reduces the potential for errors.

If you’re tempted to declare a method protected, it could be that the class isn’t designed properly. Could there be a way to factor the class better, and expose only the meaningful attributes?

Note that protected attributes should be regarded in the same way as the public interface because any method using an overridable attribute can be affected by inheritance. Therefore, making attributes protected can actually make it harder to inherit from the class. Thus, overridable attributes need documentation of all uses of them in other methods, meaning that you have to document more how a method works, instead of what it does.

And of course, you can never take anything out of your public API (which includes protected attributes), unless you really want to annoy users of the class who have already extended it.

In my opinion, it’s shortsighted to refrain from using private attributes. In the long run that will cause problems and make development harder. Using protected more will not make inheritance easier but it will make more inheritance possible. And the cost of that is potential for more errors.

Copyright © 2023 by Brandon Savage. All rights reserved.