Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I've written this small class in Pyhon that wraps bound methods but does not prevent the deletion of self. Do you have any though on my code? Do you think I handle errors appropriately? Is it missing anything, what should be done to make it more robust? Is the documentation clear enough?

[This code has be edited per the reviews below. Older versions can be seen in the history.]

import weakref

class WeakBoundMethod:
    """
    Wrapper around a method bound to a class instance. As opposed to bare
    bound methods, it holds only a weak reference to the `self` object,
    allowing it to be deleted.

    This can be useful when implementing certain kinds of systems that
    manage callback functions, such as an event manager.

    """
    def __init__(self, meth):
        """
        Initializes the class instance. It should be ensured that methods
        passed through the `meth` parameter are always bound methods. Static
        methods and free functions will produce an `AttributeError`.

        """
        self._self = weakref.ref(meth.__self__)
        self._func = meth.__func__

    def __call__(self, *args, **kw):
        """
        Calls the bound method and returns whatever object the method returns.
        Any arguments passed to this will also be forwarded to the method.

        Exceptions raised by the called bound method are not caught. They
        should be handled by the caller of this `WeakBoundMethod` object.

        Calling this on objects that have been collected will result in
        a `ReferenceError` being raised.

        """
        _self = self._self()

        if _self is None:
            raise ReferenceError('Bound method called on deleted object.')

        return self._func(_self, *args, **kw)

    def __eq__(self, other):
        """
        For two objects to be equal, the `self` and `func` attributes of
        the bound methods must have the same ids, respectivelly.

        A third condition must also be met: the `self` of the this bound
        method must not have been collected. This immitates the behavior
        of the standard `weakref.ref` type.

        """
        _self = self._self()
        return _self and _self is other._self() and self._func is other._func

@Winston Ewert's review

  1. New-style class
    I am using Python 3 atm, but if I need to use Python 2, I'll make it a new style class.

  2. Assert in __init__
    I also wondered about whether that assert is any use. I kept it because it gives a more explicit error message, but I might remove it after all.

  3. Assert in __call__
    Initially (see revision), I did raise a ReferenceError as you also suggested. Here's why I decided to change that to an assert:

    a) Ideally, the user of this class shouldn't care if it was given a bare bound method, a free function, or a WeakBoundMethod. If I raise a ReferenceError, then the user better be handling it, which means he has to know that he might get passed a WeakBoundMethod.

    b) Users who do want to couple with WeakBoundMethod can instead use the alive() function to determine if the functor is okay to call. (I admit, though, that this implementation has some limitations.)

    c) Because __call__ does not check if the weak reference is still alive (only in the debug version it does), programmers who can ensure that the bound method never gets called for collected objects through their program architecture, don't have to pay the performance price of checking on every single call. (This is also my case.)

share|improve this question
add comment

1 Answer

up vote 3 down vote accepted
class WeakBoundMethod:

I suggest making it a new-style class by inheriting from object.

    assert (hasattr(meth, '__func__') and hasattr(meth, '__self__')),\
           'Object is not a bound method.'

Don't do this. Just let the invalid parameter types raise attribute errors when you try to fetch the __func__ and __self__. You don't gain anything by checking them beforehand.

assert self.alive(), 'Bound method called on deleted object.'

Raising an Assertion here is a bad choice. Assertions are for thing that should never happen, but having the underlying self object cleaned up doesn't really count. Raise an exception like weakref.ReferenceError. That way a caller can reasonably catch the error.

The documentation is very clear, well done.

EDIT

    try:
        return self._func(self._self(), *args, **kw)
    except Exception as e:
        raise e

Why are you catching an exception only to rethrow it? That's really pointless.

I'd write the whole function as:

def __call__(self, *args, **kw):
    _self = self._self()
    if _self is None:
        raise weakref.ReferenceError()

    return self._func(_self, *args, **kw)
share|improve this answer
    
Thank you very much for your review. My reply is a bit long, so I added it as an edit to the OP. Please tell me what you think. –  Paul Sep 4 '11 at 13:40
1  
@Paul, 1: okay, I just assume everyone still uses Python 2.x. 3: a) The fact that you raise an exception doesn't mean it has to be handled. If the client code doesn't care it can ignore the exception just like it ignores the assertion exception. b) In Python asking for forgiveness is preferred to asking for permission. c) The performance difference is negligible especially if you refactor the code a bit. (I'll add what I mean as an edit) –  Winston Ewert Sep 4 '11 at 13:49
    
Thanks. I edited the code like you recommended and I also removed the alive() function since — per your point that in Python it's better to ask for forgiveness — it was not pythonic. :) –  Paul Sep 4 '11 at 14:13
1  
@Paul, you are now dereferencing self._self() twice in the __call__ function. –  Winston Ewert Sep 4 '11 at 14:50
    
Didn't notice that. –  Paul Sep 4 '11 at 14:52
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.