The following is an excerpt from a draft version of Do This, Not That: Object Oriented Development. Sign up today to be the first to get a copy this week!
A few weeks ago, I was tasked with integrating a library that was designed by someone else. This library was intended to access APIs and return the data so that it could be used by my application. This seemed straightforward enough, except that the API I was working with had a few quirks, namely that it interpreted the query string directly, and so it was possible to have a query string similar to this:
Obviously, this is “non standard” in the way of API development, but it was the method this API used to access multiple values of the same parameter.
Unfortuantely, the developer of the library didn’t allow this kind of a construction. They (most of the time correctly) assumed that you would have only one value per param, and so they took an array of key-value pairs. If you wanted more than one value for a given parameter, you provided an array as the value for that key, and it interpreted that into a standard format.
“Easy enough,” I thought to myself. “I’ll just extend the class, override the method, and move on.” Oh, if only it was that easy!
The method that needed to be overridden was marked private by the developer. Though there was no obvious reason why the method should be private, there it was, and to make matters worse, since it’s an external library, I could not simply change the visibility marker myself.
PHP’s Visibility Markers Explained
In PHP, much like in Java, there are visibility markers available to API developers. The most basic (and the default) marker in PHP is “public” – this visibility level is assigned to any method or property that is not given one of the others. Public does not need to be declared, but I recommend declaring it to be explicit.
<?php
class TestObject {
public function testFunc() {
echo "Hello World!";
}
}
class ChildObject extends TestObject {
public function testFunc() {
echo "Hello Reader!";
}
}
// Original object
$obj = new TestObject();
$obj->testFunc(); // Hello World!
// Child object
$obj = new ChildObject();
$obj->testFunc(); // Hello Reader!
?>
We can also designate methods as “protected”, which means the method can be seen, used and overridden by child classes but cannot be seen or accessed from outside the object. Protected methods are useful for internal methods that relate to the particular object but do not need to be shown outside the object. They also allow you to easily make changes to the internal API without rewriting a lot of external code.
<?php
class TestObject {
protected function testFunc() {
echo "Hello World!";
}
public function getData() {
return $this->testFunc();
}
}
class ChildObject extends TestObject {
protected function testFunc() {
echo "Hello Reader!";
}
}
// Original object
$obj = new TestObject();
$obj->testFunc(); // Error
// Original object
$obj = new TestObject();
$obj->getData(); // Hello World!
// Child object
$obj = new ChildObject();
$obj->testFunc(); // Error
// Child object
$obj = new ChildObject();
$obj->getData(); // Hello Reader!
?>
Finally, PHP allows for methods to be marked as a visibility of “private”. These methods are internal methods only, and cannot be seen, used or overridden by subclasses.
PHP has two additional designations for methods as well: final and abstract. A final method is one that has been defined as not being extensible by child classes, but can be used either internally or externally (depending on the visibility setting). Private methods are automatically considered “final”. Classes can also be marked final, which prevents the entire class from being extended.
Abstract methods are methods that are not defined in the present class, and are intended to be extended later on by the developer. Marking a method as abstract means that it has no functional content. It also requires that the class be marked abstract; that class cannot be instantiated directly and instead must be extended by a child class. Abstract methods cannot be private or final.
The Problem With Private Methods
Many developers believe that the internal methods of their objects should be private. While this thought process is understandable, it’s also wrong. Though you may wish to prevent internal methods from being externally accessible, this only requires that they be marked “protected.”
For developers whose code will never be used by anyone outside their own development team, private methods are probably simple to correct: just change the designation. But when code is to be used by others, creating a private method that should really be extensible creates a problem, because library code shouldn’t be changed, it should be extended.
Even if a developer does not write library code, it’s still a best practice to assume that another developer might extend the code in the future. Thus, using protected methods (rather than private ones) improves the ease of extensibility later on down the line, making it easier to work with the code and less likely that a developer will need to implement their own solution.
Why Not Make Everything Public?
In other languages like Python, there is no enforced concept of a “protected” method. Instead, every method is public, and there are conventions about marking methods with underscores to denote internal methods that shouldn’t be used by other objects.
The problem with marking every method as public is that the public API of an object is automatically presumed to be available for external use. This is especially true when visibility operators exist. While it is still often the convention in PHP code to mark methods with an underscore when they are protected (this started in PHP 4 when there were no visibility operators), marking all methods as public makes it impossible to maintain the API without making major changes.
<?php
// This object is difficult to change between versions, because anyone calling
// UnchangableObject::_internalCall() expects the result to remain the same.
class UnchangableObject {
public function externalCall() {
return $this->_internalCall();
}
public function _internalCall() {
echo "Hello World!";
}
}
// This object is easy to change between versions, because everyone has to
// call the public method.
class ChangableObject {
public function externalCall() {
return $this->_internalCall();
}
protected function _internalCall() {
echo "Hello World!";
}
}
?>
Instead, it is better to have methods marked as public have consistent arguments, argument orders, and return values throughout releases. For example, a method called “returnDataSet” should consistently accept the same arguments (and in later versions, new arguments that have sane defaults) and return a data structure that is the same. However, the internal, protected methods can change between versions, and it is expected that these internal API elements may be unstable between minor versions.
Be A Good Citizen
One job I worked, I had a boss that told me I should write code as though the developer coming after me would be a psychotic murderous maniac who had my address. This humorous tip reminded me to always be considerate and careful in my development. As a result, I learned never to use a private method when a protected method would do the trick. Its just good citizenship in the world of method and property visibility.
Don’t Be Left Out!
Nearly 300 people have signed up to get a first look at Do This, Not That: Object Oriented Development. Don’t be left out! Sign up to receive information about the launch of the book this week!
I always thought the argument was whether to mark methods/attributes as protected or not – because once they are marked as protected, they do become part of the published api to end users – and therefore should be treated as if they are actually public (i.e. if you remove a ‘protected’ method in the future, it’s likely it’ll annoy a user of your code).
If you expose the inner workings of your class to the world, and use convention (_) to mark them as “internals” then aren’t you at risk of someone mis-using your code and limiting your ability to change the internals over time.
ircmaxell (@ircmaxell) wrote at 12/10/2012 7:52 am:
The problem you’re experiencing here is not due to private methods. It’s due to a Single Responsibility Principal violation. Serializing the request is a separate responsibility from sending the request, which is different from processing the response. If the library was properly abstracted, you’d be able to polymorphically substitute out the serializer (using DI), and problem solved.
Practically speaking though, even in this case I’m not so sure that Private is as big of a problem as you make it. This particular case would be one where I would expect a pull request (since it’s core functionality that’s incorrect), so a patch to the original lib would be actually warranted.
Additionally your post ignores the benefits of private methods. That it makes refactoring of the original class easier, since children can’t rely upon the protected API (since there is none). That means that the only thing that’s required is the public API. The rest becomes just implementation details, which greatly simplifies refactoring, bug fixing and future feature enhancement.
So while there may be some pain, I don’t think it’s as severe as you indicate, and I think the benefits far outweigh the cons. Especially if you use a proper architecture (which would have solved your specific problem). That’s why I recommend private methods…
Brandon Savage (@brandonsavage) wrote at 12/10/2012 8:07 am:
DG, I have to disagree for this reason: marking a method as private prevents utilization of that method in subclasses at all. I believe it is incumbent upon me as a developer to ensure that any protected methods I override or depend on in subclasses work with my code; however, if you truly wish to prevent extension of a protected method you can mark it final. Zend Framework 1.x uses final in many places for this reason.
ircmaxwell, in fact in this particular library serialization was separate from execution of the request. However, the user defined all methods as either public or private; there were no protected methods that could be overridden by subclasses. I recognize that from time to time private methods may make refactoring easier, though I tend only to use them when they have a relevance only to the level of the class I’m working in, and would have no value elsewhere.
James Socol (@jamessocol) wrote at 12/10/2012 9:36 am:
A benefit of making everything public is you enable downstream users to extend, override, or just access your code in new ways you may not have thought of.
Using a method or variable conventionally marked as internal (e.g. with an underscore) communicates that the author has not committed to the stability of this API. That’s fine, if you still want to opt in. It’s like CSS prefixes, in that way.
There are downsides (users may be reluctant to upgrade if you change am internal API they depend on) but you and they may also discover whole new use cases that can end up as part of the public API.
A couple of examples from the Python/Django world are the cache._cache instance (allows you to get around cache engine agnosticism) and the django.utils module, which is filled with useful, but not guaranteed, tools.
Nicolas wrote at 12/10/2012 9:48 am:
I’am not especially for private methods they have no real adventage. In fact they are a lie to yourself so you can think you sealed your code. Let’s be honest they is no way to force anybody on your design… that in the end may be bad. They can write their own lib. On the same company this mean duplicated code… That’s not good. And mean you failed. For an external lib, that mean a lib failure with less users using it. Anyway, they can modify the code. Even by decompiling/recompiling if needed, or put a modified class before in the classpath (in java for example).
The cons is that effectively protected methods (instead of private) send like a signal “override me” to some peoples that tend to subclass a class for everycode they write.
I don’t think that even public visibility make you having to maintain the code for ever. There should be a clear entry point / facade with published service your API provide and is the only stable API your users can trust on. Providing value objects and interface for this facade make your API really independant from the outside… And I would say, if you go up to define a network protocol and provide support only for it, you are even more protected, because they no way to modify or call a method not explicitely declared under a network. Not to say that you should use network protocols everywhere, of course.
In the end, if the lib can’t fit your needs, is external and you can’t convice the lib author to add the feature you need maybe the best solution is to not use it at all. The hacks to make the lib cover your needs have long term weird implications where you may have to stick with an old lib version (that may include security holes)
Bradley Holt (@BradleyHolt) wrote at 12/10/2012 1:12 pm:
I disagree. What you’ve identified is a specific instance of a bad design. One cannot conclude from this one bad design that private methods are always bad. Similar to what ircmaxell suggested, a better design might have involved the use of the strategy pattern or, probably better for the situation described, the template method pattern. Either pattern would allow for extensibility while still using private methods.
Sven Rautenberg (@SvenRtbg) wrote at 12/10/2012 2:48 pm:
Just a few comments.
1. To your story as a whole: If you have a server side API that requires parameters like you said, and your client side library cannot support them, go get another library. :) Or live with the pain of patching it’s malfunctions.
2. A private method is NOT considered final. In fact, you can declare it final to prevent inheriting classes to create a method with the same name, whether this makes sense or not.
3. +1 from me for all comments stating that private methods are a good thing. They are not harmful at all.
You say yourself: “Finally, PHP allows for methods to be marked as a visibility of “private”. These methods are internal methods only, and cannot be seen, used or overridden by subclasses.”
So there must be a use case for private methods. Your whole case is that in your particular library there was a bug that couldn’t be fixed by inheriting and overriding a private method. Because bugs can be everywhere, including private methods, you are actually arguing that private methods should never ever be used no matter what.
You are wrong. Private methods enable implementations to be independent of the usable API. Protected methods are part of the usable API of a class. They are therefore bound to behave well regarding API changes. If a class happens to expose a protected method to the world, this method must never change again, because some obscure inheritance might take place somewhere else.
Private methods are actually a very good thing, and should be used whenever possible.
Brandon Savage (@brandonsavage) wrote at 12/10/2012 3:05 pm:
My illustration of a particular design was not meant to define all designs, but to be a specific example of a common problem. This is in the same way that a reporter writes about a specific sick person in a story on health care, or a specific unemployed person in a story on job searching.
Feel free to disagree, but you disagreeing with me doesn’t make me outright wrong.
“I’ve said it before, and I’ll say it again: I feel that frameworks and libraries should use private and final sparingly. Over the years, I’ve seen code repurposed in simply wondrous ways — largely due to keeping the code as open as possible to extension. I like to enable my users as much as possible.”
I agree with him.
Johnny wrote at 12/10/2012 9:31 pm:
I don’t think you did a good enough job selling your point. You spent most of the article just regurgitating the original php documentation.
Guilherme Blacno (@guilhermeblanco) wrote at 12/11/2012 12:15 am:
Sorry Brandon, but I share the same thoughts of Anthony.
Take a look again at MWOP’s post and he clearly explains that if you plan your code before you write it, it becomes clear (or takes hours) for others to use, extend, reuse your code without breaking initial intended behavior.
Still mentioning the same post, MWOP realized that it was his failure to look on a different perspective at the code. Instead of trying to cut, copy, paste pieces of code, it’s better to read the API or docs. =)
I’m all for private and I do tell my devs to use and abuse of private and final keywords. If the code is properly coded, there’s no need to be afraid.
Sven Rautenberg (@SvenRtbg) wrote at 12/11/2012 2:00 pm:
MWOP is also stating:
“In sum: because the class in question was marked final and had private members, I found myself forced to think critically about what I wanted to accomplish, and then thoroughly understand the public API to see how I might accomplish that task without the ability to extend.”
And:
“I’ll probably continue shying away from private and final visibility, but I do plan to experiment with it more in the future. What about you?”
Of course you are right in stating that one cannot extend private methods, that there might be a need to do so, and that it should be considered by the original developer.
The thing I strongly disagree with is your statement about “private methods considered harmful”.
DeVill wrote at 12/13/2012 6:27 am:
Well… interesting thoughts, but I think you get it all wrong! Marking things private actually aids usability. If I’m the psychotic programmer coming after you, you’d better have lot’s of small 3-4 line private methods that make no sense on their own, but help me understand the code easier :)
Luke Richards wrote at 12/13/2012 10:04 am:
I’m unsure where I sit on the subject but I did come across this relevant discussion from last year by Fabien Potencier.
That was a really long winded explanation of Open/Close principle, which has been around for a bit (decades).. but always good to see a retread of old theory; btw, you know that you’re the guy version of Lorna Jane right?
DG (@TheGingerDog) wrote at 12/10/2012 7:33 am:
ircmaxell (@ircmaxell) wrote at 12/10/2012 7:52 am:
Brandon Savage (@brandonsavage) wrote at 12/10/2012 8:07 am:
James Socol (@jamessocol) wrote at 12/10/2012 9:36 am:
Nicolas wrote at 12/10/2012 9:48 am:
Bradley Holt (@BradleyHolt) wrote at 12/10/2012 1:12 pm:
Sven Rautenberg (@SvenRtbg) wrote at 12/10/2012 2:48 pm:
Johnny wrote at 12/10/2012 9:31 pm:
Guilherme Blacno (@guilhermeblanco) wrote at 12/11/2012 12:15 am:
DeVill wrote at 12/13/2012 6:27 am:
Luke Richards wrote at 12/13/2012 10:04 am:
Christian wrote at 12/21/2012 11:59 am: