Some PEAR remarks

Hi there, I've been browsing the PEAR code to see if I would like to use the library in my own PHP programming experiences, but unfortunately I must say that - after examing the code thoroughly - I have decided I will not do that. The reasons for that decision I have written down here. Please note that I am in no way trying to attack your collective programming and design skills, even though it may seem like that at some points. These are just my personal observations, and those tend to be subjective... Also note that at some points I may sound a bit harsh. I would like to blame this on the fact that I'm not an English native speaker, so for me it can be hard sometimes to express myself properly in that language. A question in advance: why are member variables in PEAR prefixed with a '_'? Sure this is useful in languages like Java or C++ where it's hard to see which variable is a local one, an argument or a member, but in PHP this distinction is already clear: all member variables are prefixed with '$this->' inside a method. And if you use proper encapsulation, member variables are never accessed from anywhere else. (And very, very sadly, this is not the case in the PEAR library!) Let's start at the beginning: with class PEAR. This class is supposed to be the base class of all new classes (with non-static members). Examining this code led me to the following observations: - Class PEAR is large. The file 'PEAR.php' (although containing two classes) is almost 800 lines. I think that's pretty big for a base class. In my opinion, a base class should be fairly small. If you take a look at other (large) object-oriented libraries, you'll see that base classes are always pretty small, and for a reason: every other class in the system relies on it, so making it big increases overhead as well as the possibility of bugs. - Error handling is built in. That doesn't lead to a very clear separation of 'normal' code, and 'error' code. Class PEAR is bloated with error handling methods (isError, setErrorHandling, expectError, popExpect, raiseError, pushErrorHandling, popErrorHandling). In my opinion, all error handling code should be separated from the PEAR class. Even more so because PHP is an interpreted language, and any program simply hasn't got errors once it's finished, resulting in a lot of 'dead' code. With a clear separation between 'normal' code and 'error' code, the latter can be easily removed once the program is completed, which speeds it up tremendously (less code to parse, less checking to do, faster execution). - The member variable $_debug defines if the class is in debug-mode. However, there is no way this value can be set BEFORE an object of class PEAR is instantiated. (And doing it afterwards makes little sense and violates encapsulation as well.) Also, when a program is in debug-mode, it's almost always the whole program that should respond accordingly, and not just a couple of objects. This is one of the very few occassions where using a global variable ('_DEBUG_' or something) could be justified. - Using debug code slows down the program even when $_debug == false, because of the necessary if-statements. With a simple trick, this isn't necessary at all: replace the if-statements with a call to a function or method inside an assert-statement (e.g. 'assert($this->debug('PEAR constructor called'));' This function should always return true. It has a couple of advantages: 1) better separation of debugging code and normal code, 2) if assertions are disabled in php.ini, the function-call is simply skipped, adding no overhead to production code, 3) subclasses can easily add debugging code specific for that class, 4) there's no need to use a global variable as described in the previous remark. The only disadvantage of this approach is that debugging code gets a bit slower (a function call takes longer than an if-statement), but that doesn't matter much, because it's debugging code. - Subclasses of class PEAR cannot know if their superclass is in debug-mode ($_debug == true), unless they access that member variable directly. If you aim for proper encapsulation, subclasses should never be doing that. Also see the previous remark. - All in all, I think class PEAR should mainly do one thing: add destructor capability to classes. Having destructors can be very useful (although often misused). With that said, I'll move on to the database classes, as these are the most popular. Class DB is a class with static methods only, and the two most important ones (factory and connect) either return an object of the requested type, or an instance of class 'PEAR_Error'. Again, I think this is not a good separation of 'normal' code and 'error' code. Also, whenever some code calls one of these methods, it should always check whether the object returned is the one they wanted, or something else. This raises a question: how often is this done by your typical lazy programmer? And what about production-state code? That kind of code doesn't contain any programming errors, so then there's no need to check for them. Much of the error handling can be simplified by introducting 'Null' objects. These are objects of a class that simply do nothing. For example, you could have a class DB_null that is just like any DB_common-derived class, and is returned whenever a requested database class doesn't exists (when calling DB::connect). It simply returns default values from its methods instead of doing anything useful. This trick makes using the library a lot easier, and of course it applies to much more than just the DB class. Class DB is the entry point to creating connections to any kind of database. How many applications need that much abstraction? Not a lot, I think. That's not to say a class like class DB isn't a good idea, but I think a programmer should have an choice: either instantiate a class for any DBMS through class DB, or instantiate a class for a specific DBMS directly. True, the latter is still possible, but even when a class like DB_mysql is instantiated directly, class DB must still be included to get it to work, so there's little point in doing that. If class DB weren't so big (like class PEAR) and error handling was separated from the class, it would be a lot easier to get the behavior I describe here. Class DB_common, like class PEAR, is very large. Too large, if you ask me. The problem with desiging classes is always which features to put in it, and which to leave out. In my opinion, class DB_command has way too many. How many programmers will use 'sequences'? Very few, I gather (at least I certainly won't.). So put them in a separate class. If someone needs them they can easily be included, but if they're not needed, they are not loaded into memory. Other examples are the methods limitQuery, getOne and getRow. That's not to say these methods don't come in handy, but I don't think they should be in a class that emphasizes on defining an interface for creating database connections (much less using them). Why not put them in a separate utility class or something? The classes will become a lot smaller, simpler, and easier to understand. If I - as a user of the PEAR library - wish to use PEAR to create a connection with a MySQL database, the following files will be included: 'PEAR.php', 'DB.php', 'DB/common.php' and 'DB/mysql.php'. That's 793 + 874 + 1281 + 847 = 3795 lines of code (in PHP 4.2RC4). For fairly trivial tasks like making a database connection and executing SQL queries, I think that's a bit much... It is very clear that the DB classes are derived from the Perl DB classes. I find that a shame. Perl was never meant to be used as a complete programming environment, even though it has grown to become one. (On I sidenote, I seriously question the mental health of anybody who uses Perl for more than simple scripting...) I think it's better to look at programming languages with a good OO design, like Smalltalk or Java, to see how object-oriented libraries should be written. Learning OO from Perl is, in my opinion, a bit like learning to ride a bicycle from someone who has been in a wheelchair his whole life. Finally, I'd like to point out some other thing regarding PEAR: lack or reuse. One of the most important features of object-oriented programming is that code can easily be reused. With PEAR, this is not the case. I find that OO is mainly used to define class interfaces, and not to make reuse simple. Not only should PEAR be used outside of the library (in applications) over and over again, this should also be the case for code in the library itself. The reason why this isn't possible right now is that a lot of methods are very big. By dividing those big methods in smaller ones, it's much easier to reuse them. Well, that's about all I have to say about PEAR right now. Note again that all points I made are personal, subjective observations. Most of you will probably disagree, and probably rightly so. Vincent