Futurescale, Inc. PureMVC Home

The PureMVC Framework Code at the Speed of Thought


Over 10 years of community discussion and knowledge are maintained here as a read-only archive.

New discussions should be taken up in issues on the appropriate projects at https://github.com/PureMVC

Pages: [1]
Print
Author Topic: Facade.getInstance should be a classmethod instead of a staticmethod  (Read 17926 times)
chphilli
Newbie
*
Posts: 7


View Profile Email
« on: September 17, 2009, 12:31:30 »

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?
Logged
puremvc
Global Moderator
Hero Member
*****
Posts: 2871



View Profile WWW Email
« Reply #1 on: September 17, 2009, 01:45:59 »

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>
Logged
chphilli
Newbie
*
Posts: 7


View Profile Email
« Reply #2 on: September 17, 2009, 04:44:37 »

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.)
Logged
puremvc
Global Moderator
Hero Member
*****
Posts: 2871



View Profile WWW Email
« Reply #3 on: September 18, 2009, 07:07:10 »

Again, sorry about the Python/PHP confusion  :P

-=Cliff>
Logged
chphilli
Newbie
*
Posts: 7


View Profile Email
« Reply #4 on: September 18, 2009, 09:47:45 »

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?
Logged
tobydeh
Moderator
Sr. Member
*****
Posts: 52


View Profile Email
« Reply #5 on: September 21, 2009, 04:24:26 »

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.
Logged
chphilli
Newbie
*
Posts: 7


View Profile Email
« Reply #6 on: September 21, 2009, 09:19:22 »

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
Logged
puremvc
Global Moderator
Hero Member
*****
Posts: 2871



View Profile WWW Email
« Reply #7 on: September 22, 2009, 08:23:23 »

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>
Logged
akruis
Newbie
*
Posts: 2


View Profile Email
« Reply #8 on: March 13, 2013, 09:04:35 »

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
Logged
puremvc
Global Moderator
Hero Member
*****
Posts: 2871



View Profile WWW Email
« Reply #9 on: March 13, 2013, 12:00:23 »

* 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>
Logged
Pages: [1]
Print