5
\$\begingroup\$

I'm writing a class that works with a list of objects of a specific type (which is defined as class attribute). The class can either append objects via the append method or receive them as an initialisation parameter. when appending, some validation code is run. In the __init__ method i need to perform the same validation, but for a list of the aforementioned objects.

My question is, is there a best practice for such operations? Currently i'm using list comprehension to rebind the input argument to a validated list, but i'm unsure of its efficiency (time and space wise).

class Part(object):
    """
    Minimal interface class representing a single outgoing message.
    Loosely resemblant of HTTP message, but intended
    in a more general scope.
    """
    allowed_methods = ['delete', 'post', 'get',
                       'put', 'patch', 'update']

    def __init__(self, action, params=None, data=None, headers=None,
                method='GET'):
        """
        :param action: A path to a resource
        :type action: ``str``

        :param method: An HTTP method such as "GET" or "POST", defaults to 'GET'
        :type method: ``str``, optional

        :param data: A body of data to send with the request
        :type data: ``unicode``, optional

        :param params: Extra parameters to add to the action
        :type params: ``dict``, optional

        :param headers: Extra headers to add to the request
        :type headers: ``dict``, optional
        """

        if not action or not isinstance(action, str):
            raise TypeError(
                'action must be non empty str')

        method = self._normalize_method(method)

        if method not in self.allowed_methods:
            raise ValueError(
                f'invalid method {method},'
                f'allowed: {self.allowed_methods}')

        self._raw = None
        self._action = action
        self._method = method
        self.headers = headers
        self.params = params
        self.data = data

    def __repr__(self):
        return (f'<{self.__class__.__name__}>('
                f'method:{self.method}, '
                f'action:{self.action})')

    @property
    def action(self):
        if self.params:
            return encode_qs(self._action, self.params)
        else:
            return self.action

    @property
    def method(self):
        return self._format_method(self._method)

    @property
    def raw(self):
        if not self._raw:
            self._raw = self._get_raw()
        return self._raw

    def _get_raw(self):
        return self

    @staticmethod
    def _format_method(method):
        """Format the method string when accessed"""
        return method

    @staticmethod
    def _normalize_method(method):
        """Normalize the method string after acquisition"""
        return method.lower()


class BatchRequest(object):
    """docstring for BatchRequest"""

    max_len = 100
    part_cls = Part

    def __init__(self, parts=None):
        super(BatchRequest, self).__init__()
    
        if parts is None:
            self.parts = []

        if len(parts) >= self.max_len:
            raise ValueError(
                f'too many parts, maximum length: {self.max_len}')
    
        # what does happen under the hood? 
        # is the old list immediately garbage collected
        # is it better to do it in place, and if so, how?
        parts = [self.validate(p) for p in parts]
        self.parts = parts

    def validate(self, part):
        if not isinstance(part, self.part_class):
            raise TypeError(f'must be {type(self.part_cls)}, not {type(part)}')
    # does return True make more sense here?
    return part

    def attach(self, part):
        if len(self.parts) >= self.max_len:
            raise ValueError(
                f'too many parts, maximum length: {self.max_len}')
        self.append(self.validate(part))
\$\endgroup\$
1
  • \$\begingroup\$ @Reinderien sorry i didn't want to post a wall of code since it didn't seem relevant to the question, anyway i fixed it. Thank you for the input \$\endgroup\$ – darkpirate Nov 12 '20 at 17:08
2
\$\begingroup\$

On first glance, and not having particularly enough usage context, I think that most of this seems like unwarranted abstraction. I think this is a client rather than a server based on nomenclature, in which case: where are the requests calls? Why track allowed_methods when Requests does this for you? It seems like Part is just a generic wrapper for an HTTP request, which doesn't really add anything of value over what requests itself would offer.

This doesn't lend itself well to TypeError only:

    if not action or not isinstance(action, str):
        raise TypeError(
            'action must be non empty str')

You're actually checking two things: whether action is a string, if not raise a TypeError; and whether action is empty, in which case raise a ValueError.

As to your primary question around

    # what does happen under the hood? 
    # is the old list immediately garbage collected
    # is it better to do it in place, and if so, how?
    parts = [self.validate(p) for p in parts]
    self.parts = parts

That all needs to go away. Notice that validate (correctly) does not mutate the part, and its return (aside from being incorrectly indented) is not necessary at all. As such, you do not need a comprehension, do not need a new list, and only need to loop:

for part in parts:
    self.validate(part)
self.parts = parts

You ask:

does return True make more sense here?

No. What you're doing already (raising on validation failure) is fine. The validation you're doing doesn't bring much value, mind you - you're only verifying type - so it's not clear that this validation method needs to exist at all.

In summary, and barring some example usage that concretely demonstrates why any of this code needs to exist, I would delete the entire thing.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you for the very detailed answer. This code is as matter of fact an interface, by itself doesn't do much aside from providing a skeleton for the subclasses that inherits from it. The request logic is handled in a different module of the repository and the allowed methods are bound to the particular services i'm using, which only exposes a subset of the rest operations. The validation code differentiate soap and rest requests and verifies application requirements inherent to both formatting and encoding. \$\endgroup\$ – darkpirate Dec 21 '20 at 5:38
1
\$\begingroup\$

Here is the standard reference on how efficient list operations are. Generally, it's fast to add and remove things at the end of the list.

The short answer is, both are efficient--and I see no indication you need to worry about efficiency. For example, you seem to be worrying about garbage collection--of an empty list. It's not that big a deal, it's 10 bytes or less. The longer answer is, your maximum list size is 100, and it's efficient to do anything at all with 100 items--python only moves around references to items, not the content. You should stop worrying about efficiency for this problem. Why did you start? Did you see a problem? Learn how to measure how long different parts of the code are taking. Always measure, don't assume you know what's slow. Once you see a problem, and isolate it to one area of the code, THEN try to speed it up.

You should pick how to build your API based on how readable and safe it is when you use it. If you see a compelling reason that's in conflict with readability, you should try to think hard and figure out a way to get both. For example, a common option might be to offer a version of 'attach' which takes a list of parts, rather than just one, to improve efficiency. Only as a distant third options should you change your API based on efficiency.

\$\endgroup\$

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.