Re: Some PEAR remarks

On Thu, 18 Apr 2002 05:33:01 +0200, Tomas V.V.Cox wrote: > Hi Vincent, Right back at ya! :-) >> A question in advance: why are member variables in PEAR prefixed with a >> '_'? > > Just a way to make more clear to the user: "this property is private, > please do not use it". Let's drop this subject, okay? I really understand your points, but instead of your saying, I use my own: "Every property (member variable) is private, do not use it". That makes using '_' unnecessary... Anyway, this was discussed earlier, and I guess we'll simply never agree. >> 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!) > > Could you tell where in the PEAR library? Examples: class DB_Result (in DB.php), methods 'fetchRow' and 'fetchInto'. Method 'limitQuery' in class DB_common. You could say: "In 2 big classes you find only 3 occurrences, that ain't much." But in fact it is. ANY occurence is a Very Big Mistake. All baseclasses of DB_command access the parent member variables directly ($features, $dsn, and so on). And yes I know these member variables aren't prepended with a _, so they should be considered 'public'. But then they shouldn't be, because making ANY member variable public is bad design. >> 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: > > The PEAR.php class is supposed to be the *error handling* class not the > base class. Oh, okay. But then I have three (logical) questions: 1. How do I write an object that has a destructor, but no built-in error-handling? I guess I have to write it myself? 2. What is class PEAR_Error for? I mean: by the name of it it sounds like it's meant for... error handling? 3. Let me quote from 'PEAR.php': /** * Base class for other PEAR classes. Provides rudimentary * emulation of destructors. * ... I think the question speaks for itself... ;-) Also, I think there's an important difference between 'error handling' and 'error reporting'. I'll get into that again further on. >> - Subclasses of class PEAR cannot know if their superclass is in >> debug-mode ($_debug == true), > > After the explanation above that is no longer valid. We agree no? I guess we do at that. But using 'assert' statements for logging purposes might still be a good idea, don't you think? If assertions are enabled, you are technically in debug mode, and you might want to print additional (debugging) information. Just a thought. >> With that said, I'll move on to the database classes, as these are the >> most popular. > > Have you analyzed more classes? I would like to hear more comments about > other classes, please go ahead. I haven't had the time (yet). And seeing how many comments this one post produces, I might NEVER get the time :-) >> 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? > > Just one time at the top of the code: > > <?php > PEAR::setErrorHandling(PEAR_ERROR_DIE); $db = DB::connect(); $res = > $db->query(); > while ($res->fetchInto()) { > > } > ?> Are you sure about this? If I set error handling to something different, the call '$res = $db->query()' doesn't necessarily get me a DB_Result instance. It can also be a PEAR_Error. And as I stated before, it's a bad idea (and bad design) to make methods return more than one type. >> 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. > > The other day I was reading an interview to a one popular FreeBSD core > developer. He said that the success for his fixes and robust code was > putting assertions everywhere, even where is not supposed to be needed. > And I can not agree more with him. And if you talk me in the specific > case of databases, men, the number of things that can happen are too > high for not doing an extensive cheking everywhere. Again, the "bloated" > PEAR error handling will do all the job for you. No it won't. If the database server is down, there can be no error handling in PHP, only error reporting. If a disk breaks down, what can you do about it? Also: it's not a programming error (nor is it one when the database server is down). And just to be a pain in the ass: when a C(++) program is compiled with the DEBUG-flag off, all assertions are removed from the final executable... Don't get me wrong, assertions are very important, and that FreeBSD developer is probably right. But I still like to make a couple of statements on the matter: 1. Whenever you're using some programming language, you live in a sort of box you can't get out of. The size of the box depends on the language. In C(++), the box is almost the complete machine, whereas in Java it's a completely virtual one. If an error happens inside the box, you should handle it, certainly. But if an error happens outside of the box, all you can do is report it to the outside. There's no way you can access the outside, so there's nothing you can do about it anyway. 2. You can never handle all errors that can happen. You can think of many situations in which some specific error can occur, and when you think you thought of them all, there's always one more. 3. Care should be taken that the same error isn't checked for (not handled!) more than once. This may sound like a silly proposition, but it actually happens all the time, especially in lower-level languages like C++: class FancyThing { private: void doSomethingVeryCool(Thing * thing) { assert(thing != NULL); thing->doFancyTrick(); }; public: void doSomethingCool(Thing * thing) { assert(thing != NULL); doSomethingVeryCool(thing); }; }; There's nothing explicitly wrong with code like this (except that passing by constant reference (const Thing& thing) is probably better), but it's overkill: the private method 'doSomethingVeryCool' is only accessible from the public methods of the class FancyThing, and they already make sure the object passed is a valid (existing) one. Leaving all assertions out is a bad idea; putting many unnecessary asserts in is bad as well because it makes the code unreadable. If I were writing the class above, I would leave out the assert in the private method. For me that's a signal: "The pointer to 'thing' isn't checked, so we can be absolutely sure it's valid.". But we can argue about this for ages. In short: error handling is important, but you should only handle what you can handle, and you should try not to check each and every error many times. In PHP that means I try to create a database connection at the start of the script, and bail out if it couldn't be created. In the remainder of the script I simply assume it's valid. I know the database server can crash in mid-run, but what can I do about it? (except reporting it)? The database server is 'outside the box', and it's somebody else's responsibility to detect and repair serious errors with it. I think that many PHP programmers with a background in other languages (C) tend to do too much checking. PHP is not C, so should you program in the same way? I think not. All that you'll be doing is replicating code: the PHP libraries are all written in C, and you can be pretty sure they make these assertions for you. >> 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. > > I take that as your personal situation. I use 6 or 7 different databases > each day, and beleive me, I do need such abstraction. Okay, then you're one of few! :-) And a question: do you access those 6 or 7 different database in the same application, or in 6/7 separate ones? In the latter case, my argument holds. Seriously, as I said earlier, I think it's very good that a thing like this is possible, but it should be easy to create a database connection with one specific system as well. (As you agreed to below :-)) >> 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. > > This is true, and we have already discussed that (btw is not as easy as > you think if you want to preserve the speed and usability). We will > change that in the future. Preserving speed isn't a problem, I think. Personally I think the DB classes can be much faster than they are now: eliminating all that duplicated code might be a good start... As a library implementor, you can never be sure what kind of specific usability your users want. To solve that you can either put all features you can possible think of in the code, or just those that are absolutely necessary. In libraries, you should always go for the latter. If you don't, you'll end up with big, ugly, bloated classes with lots of small methods that do more or less the same things a little bit different. A classic example is a C++ string library that had all sorts of features that could possibly come in handy: reversion, capitalization, you name it. As a result, the simple program: #include <iostream> #include "bigstring.h" int main(int argc, char * argv[]) { bigstring hello("Hello world!"); std::cout << hello << std::endl; } ...ended up as an executable of 400 kilobytes. Doing the same thing with the built-in string (char[]) results in an executable of under 4 kB. Go figure. In this light, the methods 'getOne' and 'getAll' (for example) should be removed from the library. They do not add a new feature, they only make using an already existing (combination of) feature(s) a bit easier. As such, they should not be in a base class of a complete hierarchy, but in a separate helper class instead. >> 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. > > Sorry if I don't catch this point. You can't reuse them because they are > big? And example could help me here. Take a look at the methods 'getRow', 'getCol', 'getAssoc' and 'getAll' in class DB_common. They are very much alike. The duplicate code in the methods can be refactored into separate (private/protected) methods, making all methods a lot shorter and easier to understand. Also, if someone wants to add a new kind of 'get'-method this can be quite easy, because it's possible to reuse a lot of the code that's already there: the private or protected methods. In that same sense, adding a new feature to a class for ease of programming ('getOne' and 'getAll') is pretty simple. They can be implemented in a subclass or helper class by writing three or four statements, instead of the pages of code they take up now. >> 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. > > Well I mainly disagree with most of your points. I guess that you > haven't really tried to use PEAR and the bounch of things it provides. > Hope you have understood my explanations. Of course you disagree: if the PEAR developers agreed with my points, PEAR wouldn't look as it did now :-) All I try to do is give my point of view on the library. Of course I can be wrong (and I probably am in many cases). Hopefully however, we all learn something from discussions like these. If you don't, then it might just be me! Vincent