2
\$\begingroup\$

I need a custom exception for network errors that holds a related exception and more than one URL (the first one as the used service has its URL but it indirectly calls other URLs), so I wrote this:

class ExtendedNetworkError(Exception):

    def __init__(self, reason, *args, **kwargs):
        kwargs_copy = dict(kwargs)
        main_url, related_exception = self._move_kwargs(kwargs_copy)
        Exception.__init__(self, reason, **kwargs_copy)
        self.reason = reason
        self.main_url = main_url
        self.related_exception = related_exception

    def _move_kwargs(self, kwargs):
        moved = []
        for kwarg in ['main_url', 'related_exception']:
            moved.append(kwargs.get(kwarg))
            if kwarg in kwargs:
                del kwargs[kwarg]
        return tuple(moved)

    def as_dict(self):
        d = OrderedDict()
        for attr in ['reason', 'main_url', 'related_exception']:
            d[attr] = self.__dict__.get(attr)
        return d

Problems? Improvements?

\$\endgroup\$
  • \$\begingroup\$ Is this intended to be run in python 2, 3 or both? \$\endgroup\$ – 409_Conflict May 12 '16 at 19:18
4
\$\begingroup\$

Let's go bottom-to-top:

  • d[attr] = self.__dict__.get(attr)
    

    Manipulating the __dict__ of an object is rarely necessary as it is often cleaner to use getattr or setattr instead. Here, the exact replacement would be d[attr] = getattr(self, attr, None) because dict.get default to None itself. However, here you know in advance that all 3 attributes are set so you could d[attr] = getattr(self, attr) instead.

  • def as_dict(self):
    

    You can turn that initialization loop into a generator expression and feed that to the OrderedDict constructor. Doing so you’re not modifying d after building it so you could directly return it:

    def as_dict(self):
        return OrderedDict((attr, getattr(self, attr)) for attr in ['reason', 'main_url', 'related_exception'])
    
  • def _move_kwargs(self, kwargs):
    

    So… you need to get some values out of a dictonary and remove the corresponding entry afterwards: you should look into the dict.pop method:

    def _move_kwargs(self, kwargs):
         return kwargs.pop('main_url', None), kwargs.pop('related_exception', None)
    

    In fact, it would feel cleaner to do these operations directly into __init__.

  • kwargs_copy = dict(kwargs)
    

    You do not need to copy that dictionary to work with parameter value. Deleting or poping items from this dictionary is fine.

  • Exception.__init__(self, reason, **kwargs_copy)
    

    You should ask super to call the super class __init__ method for you. This is not so much of a difference with this code but it's a good habit to have, especially if you happen to need multiple inheritance or complex inheritance scheme.

    super(ExtendedNetworkError, self).__init__(reason, **kwargs)
    

    or, using Python 3, you can simplify the call to:

    super().__init__(reason, **kwargs)
    

So far, the code has been reduced to:

class ExtendedNetworkError(Exception):
    def __init__(self, reason, *args, **kwargs):
        self.reason = reason
        self.main_url = kwargs.pop('main_url', None)
        self.related_exception = kwargs.pop('related_exception', None)
        super().__init__(reason, **kwargs)

    def as_dict(self):
        return = OrderedDict(
            (attr, getattr(self, attr))
            for attr in ['reason', 'main_url', 'related_exception']
        )

Now there is this *args parameter in __init__. Since you’re not using it, you should remove it. This will force the need to use named parameter when specifying more than the reason.

Last thing you can do to improve the code is to add some docstrings explaining what this class is about.

\$\endgroup\$
  • \$\begingroup\$ Thank you for extended explanation, although one small doubt I have here is kwargs.pop() - doing this is side effect visible outside of creating exception object after all and there is tiny, remote possibility that somebody has just passed actual dict instead of using keyword args. That is why I was making a copy of kwargs. \$\endgroup\$ – LetMeSOThat4U May 17 '16 at 16:44
  • \$\begingroup\$ @LetMeSOThat4U This is not how this works. **kwargs notation is "already a copy" of anything passed as parameters. \$\endgroup\$ – 409_Conflict May 17 '16 at 18:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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