Stefan Priebsch, one of the guys who did the Habari code review of Habari at php|tek, has forwarded some notes designed to be read in conjunction with the slides from the session, which are available at available at http://www.slideshare.net/sebastian_bergmann/php-code-review-1464932
Forwarded for posterity and discussion. Thanks Stefan. 2009/7/31 Stefan Priebsch <[email protected]>: > index.php (UNIT_TEST) > > We see a coupling issue here. Unit tests should not require a complex > test environment. We doubt that it does make sense to set up "most" of > the application to run unit tests. > > What is more, at the time when this line is executed, a lot of > initialisations have already been made, and more work will likely be > done. It is hard to understand (or predict) for somebody writing "unit > tests" how the environment actually looks like, and what they can rely on. > > Any changes to index.php have a high potential of breaking unit tests. > > If certain unit tests require an environment that is "just a little > different", how should they set it up? > > > index.php (SuperGlobal) > > We think it is a very good idea to use objects instead of superglobals, > because this decouples the application code from the global environment. > This also makes it a lot easier to hand-craft input data, for example > for unit tests. It's also a very good idea to not use $_REQUEST. But why > overwrite the actual superglobals? > > Depending on whether process_gps() has already been executed or not, the > superglobals will either contain regular arrays or objects, which we > think is a bad design decision. You should not change the type of a > variable halfway through the program flow. > > We have seen code that explicitly unsets the superglobals to make it > really clear to programmers that they are not supposed to use them. > > > superglobal.php > > We think that reverting magic quotes should not happen in the same > method than creating superglobal objects, because it's really a > different concern. > > There is no way of telling that "work has already been done". Why not > use a return value to indicate that? Or better, throw an exception if > work has already been done, and thus force programmers to make sure they > only call the method once. > > Why not consistently wrap all superglobals into objects and also convert > $_COOKIE, $_ENV, and $_FILES? > > What is the benefit of converting superglobals to objects if these > objects implement ArrayIterators? You can use either like an array, > which in turn means that you never know which you are using. That means > at any point in the application, you can't tell whether you work with > escaped or unescaped data. > > The static variable $revert could be implemented as a static class member. > > > session.php (destroy) > > The session class is tightly coupled to a database. It would be better > design to have the database access in a separate class. > > Testing session requires a database. > > The function always returns true. There is no way of telling that the > session could not be destroyed (unless DB throws an exception, which > however is an exception that should be caught in the destroy() method). > > Why is destroy() a static method? You cannot swap out Session for > something else (unless you use PHP 5.3's new feature allowing you to use > variables as class names in static calls) for testing. > > > session.php (remove_error) > > Most coding standards require method names to be in camel case. > > There is no reason to check for the unset result, it will always work. > Line 353 (let aside the fact that it will always return true anyway) is > unneccesarily hard to read. A simple return !isset(...) would suffice. > > > singletion.php > > Using Singletons is discouraged by most pattern advocates nowadays. > While it still can make sense to use a selected few singletons, does > this really justify creating a base class, especially if it is not easy > and intuitively to use? > > The method is static and cannot be called through inheritance, since > polymporphism (selecting at runtime which method to call) does not take > place for static calls. Either you call Subclass::instance or you call > Singleton::instance. > > Why use an error and not an exception? > > Explicitly returning null is not necessary, since PHP does that by default. -- Michael C. Harris, School of CS&IT, RMIT University http://twofishcreative.com/michael/blog IRC: michaeltwofish #habari --~--~---------~--~----~------------~-------~--~----~ To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, visit this group at http://groups.google.com/group/habari-dev -~----------~----~----~----~------~----~------~--~---
