CODEWITHSUNDEEP
python
kkpattern
kkpattern

Reputation: 968

Python: is using decorator to change method arguments a bad thing?

I implemented a decorator to change a class method's arguments in this way:

def some_decorator(class_method):
    def wrapper(self, *args, **kargs):
        if self._current_view = self.WEAPON:
            items = self._weapons
        elif self._current_view = self.OTHER:
            items = self._others
        for item_id, item in items.iteritems():
            class_method(self, item, *args, **kargs)
        items_to_remove = []
        for item_id, each_item in items.iteritems:
            if each_item.out_dated():
                item_to_remove.append(item_id)
        for item_id in items_to_remove:
            del items[item_id]
    return wrapper

class SomeClass(object):
    @some_decorator
    def update_status(self, item, status):
        item.update_status(status)

    @some_decorator
    def refresh(self, item):
        item.refresh()

The main purpose of the some_decorator is automatically call method on every item of SomeClass then do some cleaning. Since there may be many methods I need to call on the items, I don't want to repeatedly write the for loop and the clean_items code.

Without the decorator, the SomeClass will be like:

class SomeClass(object):
    def update_status(self, status):
        if self._current_view = self.WEAPON:
            items = self._weapons
        elif self._current_view = self.OTHER:
            items = self._others
        for item_id, item in items.iteritems():
            item.update_status(status)
        items_to_remove = []
        for item_id, each_item in items.iteritems:
            if each_item.out_dated():
                item_to_remove.append(item_id)
        for item_id in items_to_remove:
            del items[item_id]

    @some_decorator
    def refresh(self):
        if self._current_view = self.WEAPON:
            items = self._weapons
        elif self._current_view = self.OTHER:
            items = self._others
        for item_id, item in items.iteritems():
            item.refresh()
        items_to_remove = []
        for item_id, each_item in items.iteritems:
            if each_item.out_dated():
                item_to_remove.append(item_id)
        for item_id in items_to_remove:
            del items[item_id]

When I actually the methods, I will do:

a = SomeClass()
a.update_status(1)
a.refresh()

Here is the problem, the parameters I pass to update_status is different from the arguments of the declaration of update_status, the item is missed since is automatically passed by the some_decorator. I wonder if it's a bad thing since it may cause confusion when other programmers see it.

If it's indeed a very bad pattern, are there any other pattern can do the same thing for me without causing confusion?

Upvotes: 4

Views: 235

Answers (2)

ely
ely

Reputation: 77484

I think even a boring module-level function would be a strong choice here. It doesn't seem that these operations that happen to your class instances really has any good reason to "belong" to the class as a class method at all.

def update_status(some_item_haver, new_status):
    for item in some_item_haver.items:
        item.update_status(new_status)

Now, it could be used with many different classes, even ones whose methods would be an ill-advised pain to modify with a decorator, like a third party class from a library someone else wrote.

a = SomeClass()
b = SomeChildOfA()
c = SomeThirdPartyThingAlsoWithItems()

update_status(a, "beep")
update_status(b, "bop")
update_status(c, "Vote for Bernie Sanders!")

Edit after the update

class SomeClass(object):
    # __init__ and other stuff
    #...

    def helper(self, func, *args, **kwargs):
        if self._current_view == self.WEAPON:
            items = self._weapons
        elif self._current_view == self.OTHER:
            items = self._others


        # The abstract part
        for item_id, item in items.iteritems():
            getattr(item, func)(*args, **kwargs)


        items_to_remove = []
        for item_id, each_item in items.iteritems:
            if each_item.out_dated():
                item_to_remove.append(item_id)
        for item_id in items_to_remove:
            del items[item_id]


    def update_status(self, status):
        self.helper('update_status', status)

    def refresh(self):
        self.helper('refresh')

Upvotes: 2

lemonhead
lemonhead

Reputation: 5518

Yeah, I think in this case, more explicit is better. Why not leave the decorators off and just use for-loops in the method itself:

def update_status(self, status):
    for item in self.items:
        item.update_status(status)

def refresh(self):
    for item in self.items:
        item.refresh()

Upvotes: 2

Related Questions