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 Python that wraps bound methods but does not prevent the deletion of self.

  1. Do you have any thoughts on my code?
  2. Do you think I handle errors appropriately?
  3. Is it missing anything?
  4. What should be done to make it more robust?
  5. Is the documentation clear enough?

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 `AssertionError`.

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

        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.

        In case an exception is raised by the bound method, it will be
        caught and thrown again to the caller of this `WeakBoundMethod` object.

        Calling this on objects that have been collected will result in
        an `AssertionError` being raised.

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

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

    def alive(self):
        """
        Checks whether the `self` object the method is bound to has
        been collected.

        """
        return self._self() is not None
share|improve this question
    
Post rolled back as the original code should remain intact. Updated code should also not be appended as it could complicate the review process. –  Jamal May 9 at 2:00
    
This is purely aesthetic consideration, but personally I really dislike linebreaks after the """. I know some people definitely do prefer them. My own thinking on whitespace has evolved -- initially I was rather influenced by this analysis of John Carmack's Doom code, which points out how vertically compact the source is. For a while I was writing some very dense javascript. :) But I've mostly come around to the more pythonic style, except for those triple double quotes. –  LiavK May 9 at 2:12

1 Answer 1

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

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.