4
\$\begingroup\$

For an assignment (not school), I was supposed to make a "web browser" using Scapy.

While storing the response, I initially thought of

# got an array of packets using sniff() named responses
ans = ""
for pack in responses:
    ans += pack[Raw].load

But that clearly doesn't work if one of the ACKs gets lost or with any of that kind of edge cases where I receive more than one of the same packet or where I receive them in the wrong order. So I wrote a "buffer" class to help handle that.

Code:

class MessageBuffer:
    """
    Used to store a buffer or message
    """
    def __init__(self, init_key):
        """
        Creates a MessageBuffer object that will hold a buffer
        containing all the message that will be received.
        :param init_key: Initial sequence number
        """
        self._buffer = []
        self._init_key = init_key
        self.unsafe = False # Can be set to be able to convert incomplete buffer

    def __setitem__(self, key, value):
        """
        Adds a message into the buffer, automatically extending the storage if needed.
        :param key: the starting point
        :param value: the string to insert
        """
        key -= self._init_key

        if key < 0:
            raise KeyError("Key cannot be less than initial key")

        if key > len(self._buffer): # Missing values
            while key > len(self._buffer):
                self._buffer.append("") # Extend till Ok

            self._buffer.extend(value) # Add all string at the end.

        else:
            try:
                while value:
                    self._buffer[key] = value[0]
                    key += 1
                    value = value[1:]
            except IndexError: # End of self._buffer
                self._buffer.extend(value)

    def is_complete(self):
        """
        :return: True if buffer is complete, otherwise False
        """
        return "" not in self._buffer


    def __str__(self):
        """
        Converts the buffer into a string.
        """
        if not self.unsafe and not self.is_complete():
            raise Exception("Buffer isn't complete")

        return "".join(self._buffer)

I don't have particular concerns, but maybe I have some kind of bad practice or something like that in the code? Also, no usage of the class but in pseudo-python-code (since copying the whole code is pretty long and not what I'm asking about)

def handler_for_packet(packet):
    buffer[packet[TCP].seq] = packet[Raw].load
    if buffer.is_complete():
        send_ack_for_packet(packet)
\$\endgroup\$
2
  • \$\begingroup\$ @200_success quick question - why didn't I receive a notification about the edit? \$\endgroup\$
    – Amit Gold
    Jul 13, 2016 at 17:10
  • \$\begingroup\$ I'm not sure, but I suspect that tag-only edits might be handled differently. \$\endgroup\$ Jul 13, 2016 at 17:11

1 Answer 1

1
\$\begingroup\$

I'd do a complete re-write of __setitem__. The rest of you class is just design decisions. And I don't have a solid alternate to it.

But __setitem__ is bad:

  • while loops are normally un-idiomatic as you can better describe the loop with a for loop.
  • You've abused error handling, as exceptions should be somewhat exceptional, not for when you don't use for. But if you change the while value loop to a for loop, extending would be simple.
  • Slicing an array to reduce its size multiple times is slow, \$O(n^2)\$ slow, and so you shouldn't do it. And using list.pop(0) is as bad.
  • Don't raise a KeyError on an item that's using indexes. If you were using a dictionary it'd make sense, but you're not.

Instead you should extend the buffer to be as long as needed, key + len(value). And so you'd only need to add key + len(value) - len(self._buffer) amount of spaces.

After this you just need to insert the data into the buffer, and as we've guaranteed it'll be long enough, we can just set a slice of the buffer. Where the start is key and the end is key + len(value).

And so it can be:

def __setitem__(self, index, value):
    index -= self._init_index
    if index < 0:
        raise IndexError("Index cannot be less than initial index.")

    end = index + len(value)
    buff = self._buffer
    buff.extend(" " * (end - len(self._buffer)))
    buff[index:end] = value

This is better than yours as most of the processing is now in C, rather than Python, and so it should be faster. It's also \$O(n)\$ rather than \$O(n ^ 2)\$. But not only that, it's easier to read.

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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