Don’t implement a known concept differently

Published on and tagged with cakephp  software engineering

If you look at the source code of CakePHP you find sometimes a piece of code where you have to say to yourself: “Wow, that’s a cool example of a bad practice! That could be used to teach principle x!” (of course, you will find such examples in other projects, too)

I found one such example as I used the core class ClassRegistry in a scenario which could be simplified as shown below:

class A {
    function id() {
        return 'A';
    }
}
        
class B {
    function id() {
        return 'B';
    }
}

$a = new A();
$b = new B();
                        
ClassRegistry::addObject('test', $a);
ClassRegistry::addObject('test', $b);
                        
$class = ClassRegistry::getObject('test');
echo $class->id();

What will be the output of this example? As you may guess the output is not “B” but “A”. Not really what you would expect. If we look at the code of the addObject method (rev. 5671) we see the reason for this result:

/**
 * Add $object to the registry, associating it with the name $key.
 *
 * @param string $key	Key for the object in registry
 * @param mixed $object	Object to store
 * @access public
 */
function addObject($key, &$object) {
    $_this =& ClassRegistry::getInstance();
    $key = Inflector::underscore($key);
    if (array_key_exists($key, $_this->__objects) === false) {
        $_this->__objects[$key] = &$object;
    }
}

An object gets only added to the registry if the respective key is not already used. If the key already exists, nothing happens and no error is thrown. So, to be sure your object gets added, you have to call removeObject() first… I don’t know why it was implemented in this way and at the moment I can’t imagine a reason why it was realized like that.

Ok, what could be done better?

The easiest and, in my opinion, the best solution is to implement it in the same way other structures to store key/value pairs work, i.e. an object with a certain key gets overwritten if a new object is added with the same key. To get this functionality, only the check for the key existence has to be removed.

If you want to keep the functionality as it is implemented now, then I think you have to do two changes: modify the API comment so that it describes what the method really does, and throw an error if someone tries to add an object with a key that already exists.

The main lesson for me from this example is: Don’t implement a known concept differently, unless you have a very good reason to do so.

19 comments baked

  • unbeliever

    The main lesson for me from this example is: don’t forget there is ClassRegistry::isKeySet() method implemented.

  • Tarique Sani

    FWIW – for me the behavior is very correct – All that is needed is a a Warning generated when an attempt is made to overwrite a key

  • Fallenjehova

    I agree with cakebaker.

    Registries work as arrays, everyone knows how array works, why changing it?

    After all, cake advocates convention.

  • Brendon Kozlowski

    I can’t wait for the time when Cake removes PHP4 support and we can use Exceptions.

  • nate

    Interesting. The main take-away for me is that you’re advocating the ability to overwrite previously-written data, which, if you actually understood the use case for ClassRegistry (i.e. what it was originally designed for), you’d realize that such a behavior is counterproductive and not wanted.

    In fact, in this case I wouldn’t even say an overwrite condition is exception-worthy. At best I’d go for something like this: https://trac.cakephp.org/changeset/5675

    Honestly Daniel, for someone who (ostensibly) knows the core as well as you, I’d have expected better. Maybe I’m just not understanding your point here, but this post just makes it sound like you’re just nitpicking for the sake of nitpicking, and it just comes off as cluelessness.

  • Mariano Iglesias

    @nate: completely agree with your last paragraph. So long with the intention of “keep writing about CakePHP”. This blog feels more like “finding stuff to complain about CakePHP” now. Guess cake bakers will have to look for new blogs now. Thankfully there are a lot!

  • Jeff Loiselle aka phishy

    CakePHP is awesome.

  • cakebaker

    @all: Thanks for your comments!

    @Tarique: Yes, that’s a possible solution, but I hope my real use case below will show you that it isn’t the best solution.

    @Brendon: Me, too :)

    @nate: Well, I wrote in the article that I don’t know why it was realized in the way it is realized, but what I know is that it doesn’t work as it should in my use case. I have a head helper, i.e. a helper which allows you to add stuff to the head area of a page. And to test it, I wrote something like:

    function testA() {
        $view = new View(new AppController());
        $this->helper->functionA('A');
        $renderedLayout = $view->renderLayout('');
        // some assert
    }
    
    function testB() {
        $view = new View(new AppController());
        $this->helper->functionB('B');
        $renderedLayout = $view->renderLayout('');
        // some assert
    }
    

    Unfortunately, this doesn’t work due to the behavior of ClassRegistry::addObject(), which is called in the constructor of the view class. To make the tests work I have to add ClassRegistry::removeObject(‘view’) in my test functions. That means I have to know implementation details of ClassRegistry::addObject(), even though I never use this class directly. If it would be implemented as proposed above, then I wouldn’t have to worry about any internal details, it would simply work.

    I hope it is now clear what the problem is.

  • nate

    “it doesn’t work as it should in my use case.”

    Exactly. That’s exactly the point. This is the crux of all arguments, both yours and mine. There are several important conclusions we can draw from this one simple statement. While not all of them may be correct, at least one of them certainly is:

    (1) The use case for which ClassRegistry was/is designed is not the same as your use case, therefore:
    (2) You shouldn’t be using it the way you’re using it
    (3) You don’t understand the purpose or intent of the ClassRegistry, ergo:
    (4) You don’t understand why it does not meet your use case.

    I’ll leave you with this parting thought: a singleton is a singleton because you only ever want one of them. So it is with the ClassRegistry. It is a singleton that provides access to single instances of objects precisely because you don’t want the overhead of instantiating more than one. Therefore if you are are trying to write an object which has been previously written, then in all likelihood you’re doing something wrong.

    Also, when doing testing, you often have to deal with classes you otherwise wouldn’t have to in order to properly simulate the test conditions. C’est la vie. I’m sure we could debate the appropriate degrees of class coupling ad infinitum, but when it comes down to it, coupling is a fact of life in systems with parts that are designed to work together.

  • Tarique Sani

    @Daniel – I find myself agreeing with Nate and Mariano here. Nate has very correctly pointed out a Singleton is a Singleton is a Singleton! You still appear to be grinding an axe – forget it man and move ahead

    @Nate – returning just true or false just increases the amount of code which the app programmer has to write – trigger_error(“You dolt! Singletons serve a purpose”, E_USER_WARNING ) would be more apt ;)

  • cakebaker

    @nate, Tarique: Thanks for your comments! I have the feeling you don’t get what I try to show :|

    Let me give you an example from the field of usability: If you see a “button”, you expect it behaves like a button, i.e. that you can press it and something happens. If you can’t press it, as it is only thought as decoration, well, then you are irritated.

    The same applies for software, and that’s what I tried to show with my example. You see a class that looks like a container for key/value pairs and so you expect it behaves in a certain way. In this case, that existing stuff in the registry gets overwritten when adding an object with the same key. If that doesn’t happen, well, then you are irritated. So you can also see it as a usability issue ;-)

  • Tarique Sani

    ClassRegistry to me looks like a way to Singletonise my classes and I does so correctly. If it allowed my singletons to be overwritten so easily I would be irritated….

    If ClassRegistry was just a way to store key/value pairs I would say what a waste – why not use associative arrays instead :D

  • the_james

    I can totally see cakebakers point here: Its not the fact that the function is a singleton, or that he is using it wrong. Its the fact that it fails silently. Additionally, in all languages with a structure / collection with a key->value pair, writing a new value to an existing key will overwrite the existing value. The context is irrelevant.

    So again, the problem here is semantics and not context. Ideally, the function should throw some sort of error when an attempt to overwrite the value is made (because evidently its not supposed to be used that way). You should not have to have intimate knowledge of the cake core to use this function. It should either work like you think it should (ie operate by normal semantics), or tell you differently at runtime (by returning an error or throwing an exception). It should NOT fail silently.

  • Repsah

    @all: I have the impression that you all are right and simply failing to understand each others…

    @cakebaker: the article is very useful, but contains two big mistakes: the title and the last sentence, reading that from a developer point of view generates irritation, I think it falls nicely in your “How to defend yourself in a discussion” article, you’re attacking first in order to prevent attacks, in this case, it’s not working.

    @nate, Tarique: Daniel is not wrong when he says that people expect a collection of data to behave like other data collections, if the intended behavior is different then it should be pointed out and throw a warning or error when used in a not intended manner. A simple web developer doesn’t want to know how the class registry works, just wants to use it, if he uses in the wrong way, you have to tell them somehow.

    @cakebaker again: I think you should appreciate the change nate implemented as it is in your thought direction, maybe you could simply add that something more visible (warning, error) would help the user understand what is going on.

    @all: keep up the good work.

  • cakebaker

    @all: Thanks for your comments!

    @Tarique: It’s OT, but I think if you have to singletonise non-singleton classes then something is wrong in your design ;-)

    @the_james: Thanks for the explanation, you did it better than I did ;-)

    @Repsah: Hm, the title and the last sentence are not thought as an attack, to me they are simply the main message of the article (and not specifically directed at the CakePHP project).

    And yes, nate’s change is a move in the right direction, but as you say, something more visible would be helpful.

  • The Mullet

    Gosh, can you believe this: even after discussing this topic, dho still is not satisfied, and wastes peoples time by posting this topic in the bugtracker at the cakeforge (#3333). What did you expect, Daniel?
    Nate and others have explained this to you, so if you don’t understand the reasons, keep quiet. But there’s no case in putting this to the buglist, especially two weeks later, and also since this is not a bug.

    Once again: if all you intend is to obstruct further development, move on to rails.

  • cakebaker

    @The Mullet: Well, ticket https://trac.cakephp.org/ticket/3333 is a different issue. Sure, it is also about the semantic of code, but not related to this post. And yes, for me it is a bug if code has side-effects you don’t expect, even though not everyone may agree. The ticket for the issue described in this post is https://trac.cakephp.org/ticket/3241, and I opened it _before_ I wrote this post.

    And before you try to imply I intend to obstruct the development of CakePHP, I recommend you have a look at https://trac.cakephp.org/query?status=new&status=assigned&status=reopened&status=closed&reporter=dho&order=priority

  • Tim Daldini

    So the ClassRegistery is a place to store singletons which for example contains the current View.

    Now I’m just wondering, what else could or should be stored there? (examples?) Does it also serve as a place to store your own (application specific) singleton instances?

  • cakebaker

    @Tim: I think it is rarely used outside of the core. But that doesn’t mean it couldn’t be useful for application specific stuff. You simply have to be aware of the behavior I described in the article.

Bake a comment




(for code please use <code>...</code> [no escaping necessary])

© daniel hofstetter. Licensed under a Creative Commons License