On private vars, inheritance and API design

Hi Folks! First off, this is going to sound like flamebait, but that's not intended. I will write about a few things that I've encountered while looking through the package sources when some Package was discussed on the list, like HTTP_Upload was now. Those things should be fixed somehow (although most of them require a new major (BC-breaking) release) or at least be avoided in the future. I think this is more important than if you use 'foo'.'bar' vs 'foo' . 'bar' or "foo" vs 'foo', since those are (at least 2 and 3, 1 only partly) exposed to the users of PEAR and can be disturbing if you *use* the packages, in contrast to other CS things that only matter to those reading/changing the package sources. The points i will talk about in this mail are: 1. Violating the "private" character of methods/properties 2. Questionable inheritance trees 3. Questionable API design If i use examples for my statements, please do not take it personally if you are a maintainer of one of those packages. 1. Violating the "private" character of methods/properties It seems to be quite common to access private methods and properties in non-private context. A "pcregrep -ril '(?<!this)->_' * | grep -v test" in the pear directory listed 172 files - I know this catches some false positives too, so don't take the number as accurate. It's just there to show that it isn't an isolated case. Let me show you an example (taken from HTTP_Request): function getResponseBody() { return isset($this->_response->_body) ? $this->_response->_body : false; } $this->_response is an instance of HTTP_Response. It isn't documented if _body is meant to be protected or private, but as HTTP_Request doesn't inherit from HTTP_Response (which wouldn't make sense) it would be wrong in both cases. While this is one of the things you usually don't note if they use the public API of the package, it does effect "userland" because: a) At least one example does this too (HTTP_Request example download-progress.php, class HTTP_Request_DownloadListener, method update, third line) - so this violation is also "documented" for end-users b) If you need to access private stuff, it shows that the API is flawed - obviously you DO need to access this information from outside. An accessor method would be appropriate there. 2. Questionable inheritance trees This seems to be an isolated case, but I think the fact that it "happened" shows that there's need for a few guidelines saying things along the lines "Don't use inheritance if all you want is code inheritance and it doesn't make sense semantically". What i'm talking about are these lines in HTTP_Upload: class HTTP_Upload extends HTTP_Upload_Error class HTTP_Upload_File extends HTTP_Upload_Error So, every uploaded file is an error? Every instance of HTTP_Upload is an error? I find that very disturbing. 3. Questionable API design Another thing I stumbled upon while viewing over HTTP_Upload because of the recent thread on pear-dev is this: function setValidExtensions($exts, $mode = 'deny') If someone does: $anUploadFile->setValidExtensions(array('jpg', 'gif')); do you really think that he expects that jpg and gif are the only extensions that are *in*valid afterwards? Here's a big difference between name and functionality. From the name of the method, the second argument doesn't make much sense - it's "set valid extensions" and not "set checks for extensions" or alike, and "set valid extensions" doesn't leave much room for interpretation. I'd vote for some "Good API design" guidelines in the CS alike to http://rpa-base.rubyforge.org/wiki/wiki.cgi?GoodAPIDesign - this is for another language, but some parts are applicable for php code also. Some extracts from it: ''' Different behaviour? Different methods. Instead of using a parameter to switch between two behaviours, make two different methods. This make it easier to read the code at the point of call, it make searches through the codebase easier, and it make overriding the behaviour in a subclass much easier. Overall, this is a case of "Make your interfaces as sharp as possible". Make your interfaces as sharp as possible Each method call should do one thing, and one thing only. This makes it easy to override methods, and make it easy to understand what the method does. One way of helping with getting a method to do a single, clear thing, is to write a comment describing what function the method has before writing the method body in itself. If either the comment or the method body seems to be doing several things, implement a different concept. ''' Applied to the previous example, there should be a setValidExtensions and a setInvalidExtensions that do what setValidExtensions($x , 'allow') and setValidExtensions($x[, 'deny']) do now. IMO, guidelines of this sort are by far more important than a guideline telling you to write 'foo' instead of "foo", which the end-user probably doesn't notice at all. With kind regards, Stefan Walk