Good use of public, private and protected in OO class design

July 19th, 2010 by Ivo

It's been a while since my last post. My blog pattern these days is that I write a blog post when I have an opinion that doesn't fit in a tweet. :)

There's been a debate in the PHP community about the use of public, private and protected. Apparently the Symfony project has decided that private is Evil, and should not be used. I don't care much about Symfony as I'm not a user, but it turned to a discussion on OO theory when Stefan defended the position by claiming that you 'should have the right to extend a class's methods if it doesn't support the use case you have'.

On Twitter, things got worse. Marco Tabini mentioned private has no role in open source code insinuating that it's about who can read the code and Travis Swicegood mentioned that protected code indicates code that is in the wrong place and later that it prevents unit testing because it creates un-testable units of code.

Before I answer to those claims, let me give you an example of a class that uses public, private and protected in the way they were intended.

The Account class

 
class Account
{
    public function depositFunds($amount) {
        $this->_updateBalance($this->currentBalance()+$amount);
    }
 
    protected function _updateBalance($newbalance) {
 
        if ($this->validate($newbalance)) {
 
            $this->__updateRecord($newbalance);
            $this->__publishAccountUpdatedEvent();
        }
    }
 
    private function __updateRecord($newbalance) {
        $this->getModel()->store($newbalance);
    }
 
    ....
}
 

As the designer of this class, I have the following considerations:

  • Somebody that uses my class in an application will need to be able to withdraw and deposit funds. This is the only feature I offer to my class users; it is my class's responsibility to take care of handling the withdrawal and deposits.

  • Derived Account classes (which will follow the 'is a' scenario of inheritance and thus will probably be special types of accounts, such as savings accounts) should be able to withdraw and deposit, but can also call _updateBalance to update the balance directly. This is something I trust derived classes with.
  • The actual act of updating the underlying model is my responsibility and my responsibility alone. This method is private because I do not want other classes to call this function directly. The only way to call __updateRecord is through the protected _updateBalance call. This way I ensure that there's always validation and that notifications get send out. This makes my application more robust because the things that need to happen, will happen. I trust derived classes to update the balance, but the trust goes only so far; the actual act of updating the underlying model is my responsibility.

This process is called 'encapsulation'.

Looking back at the arguments

Now let's look again at the arguments.

  • Stefan says that he needs to be able to override things if they don't support his use case. The above class is designed to support a variety of use cases, through the interface it offers to users and inheritors. Generally a use case is defined by the public methods a class offers, and that should be sufficient. If not, the class may have been designed poorly. Inheritors can change the use case slightly, but only to the extent that they do not endanger the overall robustness of the application.

  • Marco says that private has no use in open source projects. The above example could be part of an open source project. The decision to make a method private had nothing to do with the fact that others could read the code, nor did it have anything to do with the fact that the code cannot be changed. If this is an open source package and you want to change it because in your world you do want unvalidated account updates, you can simply download the code, change the code and be happy.
  • Travis claims that 'protected' signifies code that is in the wrong place. On the contrary, in my example I've used protected and private deliberately to design class responsibility.
  • Travis also says that it is a problem for unit testing because it creates an untestable unit. I would argue that in this case, the 'unit' is the account and its public interface. A unit test should use only the public interface which is sufficient to verify whether or not the class is doing the right things, and whether it's doing those things right. If the class contains so much code that you need unit tests that test the smaller internal functions of the class, you may be looking at a class that has too much responsibilities and that should be split up.

The arguments seem to be mostly about what you can derive but the number 1 use case of access specifiers is defining what you can call. In my opinion, preventing people from using private to make deriving easier at the expense of no longer being able to specify who can call what and define class responsibilities, is wrong. In most cases where this is a problem, the actual problem is poor class design.

There are of course numerous situations where the designer of a class has made the wrong decision and made a method private that should've been protected. In such scenario's, changing it and contributing a patch seems the right thing to do. However removing the use of private from a project entirely because some can't handle the difference, seems very wrong.

Other languages

PHP is not the first language to support private, public and protected. Other languages have seen similar debates, with more or less the same underlying principles. Ruby has built in ways to circumvent private/protected/public so functions can be unit tested. C++ is the most advanced when it comes to OO design. It supports 'private inheritance' and 'protected inheritance'. See here for examples. Also it supports a 'friend' feature that allows a class to define 'friends' that can access their private members. If classes befriend their unit tests, then unit tests can access private functions.

PHP doesn't have a 'friends' feature (I nearly said 'PHP doesn't have friends' which does sound funnier but is not what I meant ;-) ) nor does it have ways to circumvent the access specifiers for unit tests. That doesn't make the concept of private and protected any less useful though.

34 Responses to “Good use of public, private and protected in OO class design”

  1. July 19, 2010 at 8:29 am, NickBelhomme said:

    I have been following the debate and must say I must agree with you on this one Ivo. Great write-up on this hot debate.

  2. July 19, 2010 at 8:54 am, Stefan said:

    Hi Ivo,

    You take only part of my opinion and discuss it. And in your situation, your protected class can also be overridden in a subclass, yet you don’t allow the __updateRecord() to be called from subclasses, thus blocking the person extending this class from actually doing the act of updating the record (or promoting copy/paste because the person has to implement the exact same functionality in the subclass).

    There is room for private, just not in open source frameworks. Frameworks are built for extension, and people extending it should have the option of overriding and using the code in the frameworks to have it match their use case.

  3. July 19, 2010 at 9:17 am, Ivo said:

    @stefan

    My derived class can update the record, but only through the safe _updateBalance interface, which is designed specifically for this purpose.

    Another way to look at it: In a year, Symfony will no longer be able to change any of its class methods. Because they are all either public or protected, there is no longer a safe way to update the internal workings of the framework without endangering every application, because every application may have overridden every method in Symfony, so every method should be kept for backwards compatibility. Encapsulation is also a way to make sure that you can update classes without breaking compatibility.

    In my example I can get rid of __updateRecord and replace it with something more efficient, as long as I keep supporting _updateBalance.

    This has nothing to do with open source; in an open source framework well-designed classes are a must I’d say even.

  4. July 19, 2010 at 9:24 am, Stephan Hochdoerfer said:

    I do not get the point of different usage of private methods in open-source and not-open-source software. Why should the license model reflect design decisions?

    I`d call or classify private methods helper methods. They help your code to become more readable by splitting huge methods apart. In most cases you do not want to change the inner workings of the private but the logic of the method calling the private method.

    Interesting point: Since you call a public or protected method how do you know of the private method? Well you do so by having a look at the code. Basically that means you`re cheating because it should not matter to you what`s going on inside the method. Just think of a design by contract. You do not care of the inner workings as long as you get the results delivered that you expect.

    Anyway good coverage Ivo!

  5. July 19, 2010 at 9:39 am, Remi Woler said:

    While I agree with you on almost everything, I believe updateRecord should be protected here. If a sub implementation needs to store data on a different model, or use a different method on the model, the implementation would have to be reimplemented.

    However, Private does have a use in OSS/Software Development.

    Let’s say we have 2 banks, ING and ABN. In your example, basic implementation is provided, suitable for all banks. Let’s say we extend that class with an Account_ING class, and add some functionality which is limited to ING only (TAN code validation maybe)? That class should be final, and those methods should be private. No other class has any business extending or overriding those methods. If you were to need another implementation to suit ABN accounts (with Random Reader validation), you can only extend the Accounts class, and not the ING class. Simply because those implementations are specific for a single purpose, and should never be extended. Any change in ING behavior should never affect ABN behavior.

    My 2 cents (euro cents, approx 5 dollar cents).

    ~RW

  6. July 19, 2010 at 9:41 am, Ivo said:

    @remi: thanks for the comment. I disagree about the protected updateRecord. If a sub implementation needs to store data differently, then inheritance is an anti-pattern. This decision belongs somewhere up the hierarchy, where you use the Account class and through dependency injection pass it a different model. Inheritance is a very weak solution to the ‘I want to change behaviour’ problem.

  7. July 19, 2010 at 9:47 am, Stefan said:

    @Ivo: But by making the _updateBalance protected you are opening it up for overriding, meaning people may be able to actually add or change the behaviour. And in this overriding method, they will most probably want to update the record as well.

    So then I’d say make it protected but final: That way people can still update the record and call that method, yet they can not override the way this is implemented.

  8. July 19, 2010 at 9:48 am, Evert said:

    imho this is mostly an academic discussion, which doesn’t have much place in PHP.

    Sure, from an academic standpoint there’s a good place for private. The code you use to argue your point would also be a typical snippet for a textbook.

    In reality you don’t know how your classes are going to be used, and how people are going to extend it. Whenever you try to predict this, there’s a bigger chance you’re guilty of over-abstraction.

    In reality it’s better to assume the extender of your class knows what they’re doing. Either they have a pretty good idea of what the effect will be of changing the behavior of your class, or they don’t.. in which case they will likely ‘screw up’ anyway.

    In the end you’ll just make it more inconvenient for people to extend and hack around your classes.

    The only place I’d use private, is if one of my functions needs to maintain some kind of internal state, and I’m 90% positive people will never care about the value.

    Pragmatism FTW. Purism has little place in PHP.

  9. July 19, 2010 at 9:52 am, Ivo said:

    @stefan: look more closely. They can override _updateBalance but since __updateRecord is private, they won’t ever be able to actually do an update except through the original _updateBalance. The only way thay can override _updateBalance is in the sensible way:

    function _updateBalance($newbalance)
    {
    doSomeMorePreparation();
    return parent::_updateBalance($newbalance);
    }

    Which again, is by design. Making it final prevents this very useful way of overriding methods.

  10. July 19, 2010 at 9:53 am, Ivo said:

    @evert ensuring internal consistency in my class so I can update the API without worrying about who has overridden any of my methods is very pragmatic.

  11. July 19, 2010 at 10:14 am, Devis said:

    Hi ivo, very good points, I’m not into Symfony but I find the road they’ve taken quite wrong.

    Regarding the possibility of testing private/protected methods, PHP “has a friend” : if someone really needs to do it, he could suggest to use ReflectionMethod::setAccessible to change the method accessibility (see http://www.php.net/manual/en/reflectionmethod.setaccessible.php and http://sebastian-bergmann.de/archives/881-Testing-Your-Privates.html )

  12. July 19, 2010 at 10:15 am, Kirstie said:

    Ivo I totally agree with your design decisions here. Too often when working on a large system or platform, our design methods are hacked to pieces because the use (or misuse) of private, protected and public thereby creating a mess of functionality and often spawns duplicate functionality. I don’t believe that good design principles affect the notion of open source at all and sensible access methods in place of a free for all is highly advisable.

  13. July 19, 2010 at 10:17 am, Devis said:

    sorry for the errors, Ivo I need a preview button :-P

  14. July 19, 2010 at 12:50 pm, Lukas said:

    @Remi: but what if the API of one of the bank changes? what if they offer a new variation of the API? what if they merge with another bank and start adopting some of their stuff? Do you really want to wait for up stream in that case when a client is breathing down your neck (and upstream is waiting for some QA on some other stuff etc)? Then it really sucks to have stuff be non extensible or requiring copy and paste.

    However, DVCS like git and sites like github.com could be considered a game changer. Suddenly forking the code, fixing the issue and keeping in sync with upstream becomes trivial. Even better your fix is also easy to submit to upstream. So the quickest way to solve your problem (aka fork) also gets your change submitted to up stream. Awesome.

    Now there are three scenarios that can happen:
    1) up stream doesnt care about your change, in which case you have to keep your fork in sync with upstream. probably more or less the same amount of work than making sure your inherited class still works if they marked everything as public to enable extending. so its a wash as to whats better .. but i think the fork is still cleaner, because eventually you might still be able to talk some sense into up stream
    2) up stream cares, but decides to fix the issue differently. in this case you can decide to drop your fork and go back to the original code, hopefully with little changes necessary (after all you were messing with internals of the object, not really with the user facing API)
    3) up stream accepts your patch, in which case you just switch back to up stream

    suddenly i am feeling that marking stuff protected and private isnt so bad after all .. as it clearily marks the extension points, clearly marks where BC breaks may occur, yet I am still in control .. if and only if the source code is made available in a DVCS. I am slowy coming around more and more.

  15. July 19, 2010 at 1:15 pm, Jordi Boggiano said:

    I fully agree that language features shouldn’t be prevented in coding standards, unless they’re deprecated/obsolete. If we follow that logic, then we can’t use Exceptions because some people mis-use them, and namespaces are quite confusing too to some novice developers.. Then we all end up using PHP4 again because it is the lowest common denominator of our community member’s knowledge? Great.

  16. July 19, 2010 at 1:39 pm, Sebs said:

    I think the PHP community matures here little later than other languages that started with OOP from day 1. There is still lots of room for improvements which probably are not yet written down somewhere.
    So I find myself quit reading tweets or posts about that topic as early as I can identify it as black/white. All this is so dependant on the team you work in, the environment you use and the public or non-publicness of the API/Class.

    Btw: I coded some loc in my life, but never had to implement a banking system. Is it the example that gives the impression this is a academic discussion?

  17. July 19, 2010 at 1:47 pm, Rick Buitenman said:

    The use of public, protected and private should reflect the *intent* of a class’ design.

    An open source framework, or any codebase for that matter, doesn’t simply become “extentable” simply by exposing it’s innards to subclasses. Hard rules like “don’t use private” are not a substitute for good design. In fact, in the real world they usually have exactly the opposite effect, because people stop thinking about how to design their classes for extendability.

    Making a conscious choice between private and protected is a very important part of the design process. I understand why certain projects, like Symfony, would consider the presence of private methods to be a potential code smell, but encouraging developers to simply skip that part of the design of the code via the coding standards seems like a very bad idea.

  18. July 19, 2010 at 3:02 pm, Lukas said:

    @Rick: nobody is arguing that it becomes magically extensible .. the argument is that it becomes hackable. when in a bind this can be a life saver in the real world, but like i argued above, if we all use DVCS, then this becomes a non issue and we can simply fork instead of hack.

  19. July 19, 2010 at 3:07 pm, Gerard said:

    I don’t know about all that, but Why oh Why oh Why does the andoid framework make the util log class final? http://developer.android.com/reference/android/util/Log.html

  20. July 19, 2010 at 3:24 pm, Stephan Hochdoerfer said:

    Even with the private method Ivo`s code is still hackable, maybe not in the way most of you would expect or want it. You just need to overwrite the protected method. If the code of the private method would not exists as separate method it would be part of the _updateBalance() method. In that case you would also overwrite that method in a subclass. So what`s the deal?

    Forking when hacking fails seems at least to me to be the wrong way, because you would need to maintain the fork, apply patches and make sure everything runs smooth. Too much extra work for so little benefit.

  21. July 19, 2010 at 3:59 pm, Lukas said:

    @Stephan: I saying that forking becomes attractive with a modern DVCS because maintaining the fork is trivial in most cases. Not anymore work than maintaining a hack and way better to get the change integrated up stream.

    Also of course you can also copy paste the given private method, but copy paste is the worst possible hack.

  22. July 19, 2010 at 4:15 pm, Ivo said:

    @stephan: you’re wrong; you could overide _updateBalance, but in your overridden version you would have no way to call the underlying __updateRecord. You can however call parent::_updateBalance which is a perfectly good use case.

  23. July 19, 2010 at 4:46 pm, Stephan Hochdoerfer said:

    @Ivo Basically we`re on the same side, maybe my example went completely i the wrong direction or you did not fully get what I meant :D

    @Lukas: I agree that c&p a private method is the worst hack ever. Although I`d recommend Dependency Injection as an alternative to be able to completely exchange the implementation without the need to fork, hack or whatever.

  24. July 19, 2010 at 5:24 pm, Vance Lucas said:

    I too chimed in on this twitter argument (@vlucas). The problem with private and final is that you’re basically saying “I know how this code will be used and extended in the future, and the code as it is right now is exactly the way it needs to be for all use cases”. That comes off to me as a pretty arrogant design that makes a lot of impossible assumptions.

    If you have a “perfect” architecture and always allow for extension by other means, then it can turn out to be okay. Pragmatically, however, I have never yet seen this be the case. In the real world, working with code that has private and final declaration almost always results in ugly hack workarounds or duplicated code. It’s one of those things that can be justified and looks good from an academic standpoint, but causes problems and headaches in the real world when used by novice and even experienced programmers alike.

  25. July 19, 2010 at 7:27 pm, Robin Speekenbrink said:

    hmmmzz… been following the discussion on the side, but Ivo, you clearly know how to simplify a delicate situation :) So please allow me to add in my 2 cents…

    In your example, i could redeclare (overload if you will) the _updateBalance() and not call the parent, but just call my own definition of __updateBalance (or __overloadedUpdateBalance, ___updateBalance whatever) just by having it private is by no means a ‘hard’ lock in that every one of your derivatives will adhere to the given standard…

    I do agree with you that some form of bottom line should be defined, but that should be defined on a per-project-basis… This shouldn’t be a discussion on _all_ PHP projects should do this or that… but MY project does this… not that.

    Again that also goes for the point of ‘forking’ instead of ‘hacking’: thats just bypassing the whole discussion. The choice of VCS or the fact that the project is OpenSource or not, has IMHO nothing to do with it… It is about the choices made by the original developer of what contract is available to you as user/developer of the application. The use is to be documented along with usecases and examples. The fact that you can peek inside the code is a plus, the fact that you can alter the actual code is just room for disaster. Forking could ba solution, but has long term issues. Just live by the contract or if you really have to change it, overload the function and stick to it… This exact problem is why the great thinkers of the past thought up the programming by contract paradigm. Define the contract, extend what you like, but keep the contract..

    The argument of unittesting seems trivial: the given example is an ideal candidate for stubbing/mocking and via dependency injection so that every part (including the privates of sending out events) could (and should be btw) easily be tested

    The fact remains: symfony made a decision. Any decision should be adhered when using the framework, just like their choice for a certain class signature, this is just part of the project. Live with it or live without it.. It’s just that simple… (or am I now the one over simplifying it…?)

    Oh and for the record, i am not a true symfony user, though a great fan of Mr. Potencier’s work! I will therefore be looking forward to his say in this… :)

  26. July 19, 2010 at 7:43 pm, eRadical said:

    I agree with Ivo… access modifiers are so unused and/or misunderstood in PHP world…

    And for those asking for “different backend” can look at “Strategy Pattern” (http://en.wikipedia.org/wiki/Strategy_pattern).

  27. July 19, 2010 at 7:59 pm, Cory Comer said:

    You have a bank, a teller, and a customer. A customer walks up to the teller… “Hello, I have this check here for $400 I would like to deposit into my bank account, here is my account number.”. The teller takes the check, looks at the back of the check to ensure it has been endorsed, maybe looks for a watermark or some other means of verifying authenticity, punches the account number into the computer and then runs the check through a little machine that scans it into the bank’s computer. The machine, owned and operated by the bank checks the serial number on the check to ensure it is correct, verifies the account number, and then sets up the withdraw and deposit. The teller hands the customer a receipt, all is right with the world.

    The teller has methods that are considered public, any customer can access them. They are abstractions and aggregates of other functionality the teller must perform in order to satisfy the expectations of the customer. The depositMoney method for example makes a call to the banks protected method verifyCheck to ensure the check has been endorsed and is in fact real. Once the teller has satisfied it’s own inherited prerequisites for the check to be deposited, they put it into the machine that the bank manages to initiate the withdraw and deposit. The teller does not know how this works in the same sense that the customer doesn’t know all the details of the verification process the teller performs.

    The bank will need to perform many actions to setup the withdraw and deposit, and these actions will no doubt change in the future. But the teller need only pass the check to the private initiateWithdrawAndDeposit method of the bank to initiate this process. The teller may incorporate their own verification methods depending on their training and expertise to ensure the check isn’t fraudulent and the customer is following the banks standards and requirements.

    In the end, the customer is only exposed to the functionality they need, in this example actually depositing a check. The teller is given discretion as to how the verification of the check happens, and the bank dictates the deposit process without exposing it to teller or customer.

    Public, protected, and private.

    Composition and encapsulation are core tenets of OOP. In this scenario, if everything had been public, and the customer had invoked a particular method of withdraw and deposit, or if the teller had done this the bank would be forced to always support that particular method for backwards compatibility. Maintaining backwards compatibility becomes an exponential liability as eventually there comes a time compatibility is usually broken, and either refactoring starts to creep in or you end up with project forking and divergent code bases.

  28. July 19, 2010 at 8:08 pm, Gargoyle said:

    Good post Ivo. I missed the original discussion, but I agree with you. I have seen a lot of examples recently in the area of database mappers and models that are implementing public __call()/__get()/__set() methods that basically end up providing a means to access private properties – thus completely removing the encapsulation!

    For those that are arguing that they cannot change the database and override __updateRecord, well that’s exactly what is intended! If you change the database then you are responsible for overriding _updateBalance and providing your own equivalent of the private _updateRecord() method that “knows” about your version of the database.

    As an example, lets assume I wanted a version of account that stored 15% of the deposits as ‘sales commission’, then I would override _updateBalance() that split off the 15%. I would then call parent::_updateBalance() with the remaining 85%, and write my own _updateCommissionRecord() that knows where to save my custom database value!

  29. July 19, 2010 at 8:14 pm, Koen said:

    Protected methods should be forbidden in open source projects. Only public and private.

  30. July 19, 2010 at 11:01 pm, Seva Lapsha said:

    What scares me out is that these points come to discussion at all. It just clarifies about level of expertise of these people in programming at all.

    First they dismiss static, then access levels… What will come next? Constructors? :) :(

  31. July 20, 2010 at 3:16 pm, FredTheSwiss said:

    Private methods are often the symptom of breaking the Demeter Law (Which is the case of your example).

    Public, private, protected has more to do with writing clean code than source code contract.

  32. July 20, 2010 at 3:23 pm, FredTheSwiss said:

    (Please do not take into account my previous and wrong message).

    Private methods are often the symptom of breaking a simple rule of thumb: if your class description contains an “AND”, your class does too much and should be broken.

    It is the case of your example: The account class record the data and validate it and publish the events.

    Public, private, protected has more to do with writing clean code than source code contract.

  33. July 20, 2010 at 7:19 pm, JW Eshuis said:

    I agree on Ivo. I think the solutions he describes is the best solution for this problem. If indeed Symphony (and sorry @stefan i don’t know it that well) only uses public or protected functions i think they will have a problem with backwards compatibility in the future.

  34. July 21, 2010 at 2:42 am, Jesse said:

    I don’t think anybody has mentioned the need for a factory. If you want to keep the api simple for a webservice for example, much of the web service description could be reflected via a Factory class, which can be used in simplifying constructors for sub-classes (1 less argument), and ultimately don’t have access to the object being created by the factory because it is private.. fair enough ??