Facade.getInstance should be a classmethod instead of a staticmethod

[chphilli]

Facade.getInstance currently has the following implementation (I have removed the docstring):

    @staticmethod
    def getInstance():
        return Facade()

This implementation requires that any subclass of Facade override the getInstance() method to make sure that the subclass is properly instantiated -- since Facade is designed to be subclassed, getInstance is effectively unusable.

Here's an alternative implementation:

    @classmethod
    def getInstance(cls):
        return cls()

This implementation removes the need for subclasses to override getInstance(), since cls will be the derived class, and the __new__ method will be called properly for both the Facade class and subclass.

Thoughts?

[puremvc]

Not having to override getInstance is nice. Having to pass in the class name changes the method signature, though. And there is a MultiCore version of the PHP port on its way. The MultiCore Facade.getInstance mehtod takes multiton key as the argument, though I suppose it could have two arguments.

However looking at this, not knowing PHP, it looks to me as if multiple calls to getInstance would return a new instance. If so, that's no kind of Singleton! We should get back the singleton instance if it exists, or create it and return it otherwise.

-=Cliff>

[chphilli]

Just to avoid any confusion, this problem is with the Python port, not the PHP port.

In the example I posted below, you don't actually have to pass in the class name -- that's done by Python for you, similar to the self variable on normal methods.

As for multiple calls, the way that the __new__ method is implemented, it will only return the existing instance, so you won't ever get multiple instances. (Which protects the Singleton assumption.)

[puremvc]

Again, sorry about the Python/PHP confusion 

-=Cliff>

[chphilli]

Okay,

I discovered that my suggestion above didn't completely fix the problems. Unfortunately, several of the base classes provided by the PureMVC pattern directly reference the Facade base class, which prevents them from properly finding a derived ApplicationFacade() class for the custom application.

I think it boils down to two problematic methods: Facade.getInstance and Facade.__new__.

The problem with Facade.getInstance is that it's a staticmethod instead of a classmethod, so it creates an instance of Facade, instead of an instance of ApplicationFacade when the class that is actually being called from is ApplicationFacade.

The problem with Facade.__new__ is that it *does* use the cls argument for storing the singleton instance (cls.instance). This means that when an ApplicationFacade is created, it is stored at ApplicationFacade.instance, and when a Facade is created, it is stored at Facade.instance. Since several classes in the framework must reference the Facade class directly (they don't and can't know anything about a custom ApplicationFacade), the framework classes get the Facade instance, while custom usage gets the ApplicationFacade instance.

To fix this problem, here's what I suggest something like this on the Facade class:

:

class Facade(object, puremvc.interfaces.IFacade):
    #...

    def __new__(cls, *args, **kwargs):
        if not Facade.instance:
            Facade.instance = object.__new__(cls, *args, **kwargs)
            Facade.instance.initializeFacade()
        return Facade.instance

    @classmethod
    def getInstance(cls):
        if Facade.instance:
            return Facade.instance

        if cls is Facade:
            raise NotImplementedError('Cannot create instances of Facade directly.')

        return cls()

    #...

This approach requires that the application call ApplicationFacade.getInstance() prior to any other part of the framework calling Facade.getInstance(), but ensures that Facade.instance will always be of type ApplicationFacade. It also frees the user from having to override either of these methods.

What do you think?

[tobydeh]

Hi Chris,

I agree that there is probably a better way of writing getInstance() but I wrote it the way I did to stay in keeping with the original AS3 documentation.

This is taken from the comment string in Facade.as:

:

     
     *    public class MyFacade extends Facade
     *    {
     *        // Notification constants. The Facade is the ideal
     *        // location for these constants, since any part
     *        // of the application participating in PureMVC
     *        // Observer Notification will know the Facade.
     *        public static const GO_COMMAND:String = "go";
     *         
     *        // Override Singleton Factory method
     *        public static function getInstance() : MyFacade {
     *            if (instance == null) instance = new MyFacade();
     *            return instance as MyFacade;
     *        }

Cliff, what's the AS3 official standing on Facade.getInstance()? Should the user always override, I've noticed some examples which use ApplicationFacade.getInstance()?

I think that the python port should stay in tune as much as possible to make use of the AS3 documentation and examples.

Toby.

[chphilli]

Hi Toby --

Thanks for getting back to me.

I don't think it's a problem that subclasses of Facade have to override getInstance() -- it's convenient that they don't necessarily have to, but if it's more in line with the PureMVC standard, there's nothing wrong with forcing the override.

However, as I noted previously, the current implementation also allows for multiple instances of Facade, which is definitely a problem. The fix I mentioned will take care of it -- as it happens, it means we don't have to override getInstance() which I just take as a bonus.

Thoughts?

--Chris

[puremvc]

Yes, you want to override the getInstance method in the Facade subclass.

There is only one place in a Standard version app that you'll be calling getInstance, and that is in your view hierarchy (or in whatever code constructs the view). We construct the view, then get the instance of the Facade subclass and call a startup convenience method on it, passing in a reference to the view hierarchy, so that the PureMVC apparatus can attach itself to the view.

Since you generally define a startup method in your Facade subclass, you want to be sure that you are not getting an instance of the super class which won't have that startup method.

-=Cliff>

[akruis]

Just for the records: if anyone needs it, an implementation of the changes proposed in this discussion is available from
https://github.com/akruis/puremvc-python-standard-framework/commit/9c94474f5fb57e30a052efab3ad8ef64bbbff7dc

[puremvc]

* Is this an API change, i.e., client code will have to change?
* Is there a significant behavioral change, i.e., something will not work as expected? - I don't think people should be relying on it allowing multiple Facades, since that's what MultiCore is for, but anything else?

Don't want to accept a pull request automatically if it might break client code, that's all.

-=Cliff>