0
\$\begingroup\$

This code is part of Construct library that converts between byte and bit representations.

from construct.lib.py3compat import *


def integer2bits(number, width):
    r"""
    Converts an integer into its binary representation in a b-string. Width is the amount of bits to generate. If width is larger than the actual amount of bits required to represent number in binary, sign-extension is used. If it's smaller, the representation is trimmed to width bits. Each bit is represented as either b'\x00' or b'\x01'. The most significant is first, big-endian. This is reverse to `bits2integer`.

    Examples:

        >>> integer2bits(19, 8)
        b'\x00\x00\x00\x01\x00\x00\x01\x01'
    """
    if width < 1:
        raise ValueError("width must be positive")
    number = int(number)
    if number < 0:
        number += 1 << width
    bits = [b"\x00"] * width
    i = width - 1
    while number and i >= 0:
        bits[i] = int2byte(number & 1)
        number >>= 1
        i -= 1
    return b"".join(bits)


def integer2bytes(number, width):
    r"""
    Converts a b-string into an integer. This is reverse to `bytes2integer`.

    Examples:

        >>> integer2bytes(19,4)
        '\x00\x00\x00\x13'
    """
    if width < 1:
        raise ValueError("width must be positive")
    number = int(number)
    if number < 0:
        number += 1 << (width * 8)
    acc = [b"\x00"] * width
    i = width - 1
    while number > 0:
        acc[i] = int2byte(number & 255)
        number >>= 8
        i -= 1
    return b"".join(acc)


def onebit2integer(b):
    if b in (b"0", b"\x00"):
        return 0
    if b in (b"1", b"\x01"):
        return 1
    raise ValueError(r"bit was not recognized as one of: 0 1 \x00 \x01")


def bits2integer(data, signed=False):
    r"""
    Converts a b-string into an integer. Both b'0' and b'\x00' are considered zero, and both b'1' and b'\x01' are considered one. Set sign to interpret the number as a 2-s complement signed integer. This is reverse to `integer2bits`.

    Examples:

        >>> bits2integer(b"\x01\x00\x00\x01\x01")
        19
        >>> bits2integer(b"10011")
        19
    """
    number = 0
    for b in iteratebytes(data):
        number = (number << 1) | onebit2integer(b)

    if signed and onebit2integer(data[0:1]):
        bias = 1 << (len(data) -1)
        return number - bias*2
    else:
        return number


def bytes2integer(data, signed=False):
    r"""
    Converts a b-string into an integer. This is reverse to `integer2bytes`.

    Examples:

        >>> bytes2integer(b'\x00\x00\x00\x13')
        19
    """
    number = 0
    for b in iterateints(data):
        number = (number << 8) | b

    if signed and byte2int(bytes2bits(data[0:1])[0:1]):
        bias = 1 << (len(data)*8 -1)
        return number - bias*2
    else:
        return number


def bytes2bits(data):
    r""" 
    Converts between bit and byte representations in b-strings.

    Example:

        >>> bytes2bits(b'ab')
        b"\x00\x01\x01\x00\x00\x00\x00\x01\x00\x01\x01\x00\x00\x00\x01\x00"
    """
    return b"".join(integer2bits(c,8) for c in iterateints(data))


def bits2bytes(data):
    r""" 
    Converts between bit and byte representations in b-strings.

    Example:

        >>> bits2bytes(b"\x00\x01\x01\x00\x00\x00\x00\x01\x00\x01\x01\x00\x00\x00\x01\x00")
        b'ab'
    """
    if len(data) & 7:
        raise ValueError("data length must be a multiple of 8")
    return b"".join(int2byte(bits2integer(data[i:i+8])) for i in range(0,len(data),8))


def swapbytes(data, linesize=8):
    r"""
    Performs an endianness swap on a b-string.

    Example:

        >>> swapbytes(b'00011011', 2)
        b'11100100'
        >>> swapbytes(b'0000000011111111', 8)
        b'1111111100000000'
    """
    if len(data) % linesize:
        raise ValueError("data length must be multiple of linesize")
    if linesize < 1:
        raise ValueError("linesize must be a positive number")
    return b"".join(data[i:i+linesize] for i in reversed(range(0,len(data),linesize)))

some of the code refers to: https://github.com/construct/construct/blob/master/construct/lib/py3compat.py

relevant test cases are here: https://github.com/construct/construct/blob/master/tests/lib/test_binary.py

\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

Your code is quite literally WET. I'd recommend that you merge integer2bits and integer2bytes into one private function. And for you to merge bits2integer and bytes2integer. This allows for you to reduce the amount of testing needed. As you now only need to test one function rather than two. If you pass to them lists of numbers rather than bytes or strings than you can simplify the internal logic too.

Starting with integer2bits and integer2bytes to make _integer_convert. You should be able to see that the first five lines are nearly identical. The only difference is how much you shift the number by. And so if we pass a variable size to the function to define the size of the data. You can change the shift to 1 << width * size.

def _integer_convert(number, width, size):
    if width < 1:
        raise ValueError("width must be positive")
    number = int(number)
    if number < 0:
        number += 1 << width * size

After this you should notice you're using a bit mask when passing number to int2byte. This mask is equal to (1 << size) - 1. You're also going through the number in chunks, where a chunk starts at i * size. And has the size of mask. This means that we can change the function to be:

def _integer_convert(number, width, size):
    if width < 1:
        raise ValueError("width must be positive")
    number = int(number)
    if number < 0:
        number += 1 << width * size
    mask = (1 << size) - 1
    acc = [b"\x00"] * width
    i = width - 1
    while number > 0:
        acc[i] = number & mask
        number >>= size
        i -= 1
    return acc

This is good and all, but you can change it to a generator comprehension. This is almost definitely slower than the current implementation. But can be easier to read, if you're familiar with comprehensions. The range that we'll be using will start at width-1, and end at 0. And so we can use range(width-1, -1, -1). After this I'd make the chunks start, and then perform the mask on the data. This allows for smaller code, through the use of comprehensions, but is likely to be slower than the above function. And terrible compared to the above with _integer_convert(0, 20, 1).

def _integer_convert(number, width, size):
    if width < 1:
        raise ValueError("width must be positive")
    number = int(number)
    if number < 0:
        number += 1 << width * size

    mask = (1 << size) - 1
    chunks = (i * size for i in range(width-1, -1, -1))
    return (((number & mask << chunk) >> chunk) for chunk in chunks)

To further improve this function I'd add doctests to it. Your current doctests are limited and are actually erroneous. And so using tuples as input and converting the iterator to a tuple as output can allow safer and more clear tests. I'd also test all aspects of the function. Some different sizes, and at least the sizes 1 and 8. Some different numbers, and a couple of different widths. But you almost definitely have to test a negative number. You don't do this at the moment. And I actually didn't change number += 1 << width to number += 1 << width * size originally and the new tests picked that up. And so these tests are already better than your old tests. Adding all these left me with the function:

def _integer_convert(number, width, size):
    """
    >>> tuple(_integer_convert(170, 8, 1))
    (1, 0, 1, 0, 1, 0, 1, 0)
    >>> tuple(_integer_convert(170, 4, 2))
    (2, 2, 2, 2)
    >>> tuple(_integer_convert(170, 4, 3))
    (0, 2, 5, 2)
    >>> tuple(_integer_convert(170, 4, 4))
    (0, 0, 10, 10)
    >>> tuple(_integer_convert(170, 4, 5))
    (0, 0, 5, 10)
    >>> tuple(_integer_convert(170, 4, 6))
    (0, 0, 2, 42)
    >>> tuple(_integer_convert(170, 4, 7))
    (0, 0, 1, 42)
    >>> tuple(_integer_convert(170, 2, 8))
    (0, 170)
    >>> tuple(_integer_convert(170, 1, 8))
    (170,)
    >>> _integer_convert(170, 0, 8)
    Traceback (most recent call last):
        ...
    ValueError: width must be positive
    >>> tuple(_integer_convert(-42, 8, 1))
    (1, 1, 0, 1, 0, 1, 1, 0)
    >>> tuple(_integer_convert(-170, 1, 8))
    (86,)
    >>> tuple(_integer_convert(-170, 2, 8))
    (255, 86)
    >>> tuple(_integer_convert(19, 8, 1))
    (0, 0, 0, 1, 0, 0, 1, 1)
    >>> tuple(_integer_convert(19, 2, 8))
    (0, 19)
    """
    if width < 1:
        raise ValueError("width must be positive")
    number = int(number)
    if number < 0:
        number += 1 << width * size

    mask = (1 << size) - 1
    chunks = (i * size for i in range(width-1, -1, -1))
    return (((number & mask << chunk) >> chunk) for chunk in chunks)

After this we should redefine integer2bits and integer2bytes, these are going to be rather WET, but writing everything twice rather than eight times is the lesser of two evils. I'd also change your doctests to work in both Python 2 and Python 3. But you can skimp on them here as you're only testing the joining of the strings/bytes. And so can become:

def integer2bits(number, width):
    r"""
    Converts an integer into its binary representation in a b-string. Width is the amount of bits to generate. If width is larger than the actual amount of bits required to represent number in binary, sign-extension is used. If it's smaller, the representation is trimmed to width bits. Each bit is represented as either b'\x00' or b'\x01'. The most significant is first, big-endian. This is reverse to `bits2integer`.

    Examples:

        >>> str(integer2bits(19, 8).decode('utf-8'))
        '\x00\x00\x00\x01\x00\x00\x01\x01'
    """
    return b"".join(int2byte(i) for i in _integer_convert(number, width, 1))


def integer2bytes(number, width):
    r"""
    Converts a b-string into an integer. This is reverse to `bytes2integer`.

    Examples:

        >>> str(integer2bytes(19, 4).decode('utf-8'))
        '\x00\x00\x00\x13'
    """
    return b"".join(int2byte(i) for i in _integer_convert(number, width, 8))

After this we should change bits2integer and bytes2integer in the same way. First I'd like to point out that (1 << (n - 1))*2 == 1 << n. The only difference is the latter doesn't error when n is 0. And so you can simplify that line. Another difference is number = (number << 1) | onebit2integer(b), instead I'd move onebit2integer out of this function. And so I'd change the for loop to iterate through iterateints(data) and change this line to (number << size) | b & mask. This allows for any size, and the special matches can be moved out of the function. Finally if we pass iterateints(data) as the data, we can simplify the check if the number is negative. This is as we can get the first number, or default to zero, next(iter(data), 0). And check if the correct bit is set, next(iter(data), 0) & 1 << (size - 1). And finally adding the inverted function doctests from _integer_convert, and some doctests for signed, we can get:

def _byte_convert(data, signed, size):
    r"""
    >>> _byte_convert((1, 0, 1, 0, 1, 0, 1, 0), False, 1)
    170
    >>> _byte_convert((2, 2, 2, 2), False, 2)
    170
    >>> _byte_convert((0, 2, 5, 2), False, 3)
    170
    >>> _byte_convert((0, 0, 10, 10), False, 4)
    170
    >>> _byte_convert((0, 0, 5, 10), False, 5)
    170
    >>> _byte_convert((0, 0, 2, 42), False, 6)
    170
    >>> _byte_convert((0, 0, 1, 42), False, 7)
    170
    >>> _byte_convert((0, 0, 0, 170), False, 8)
    170
    >>> _byte_convert((170,), False, 8)
    170
    >>> _byte_convert((1, 1, 0, 1, 0, 1, 1, 0), True, 1)
    -42
    >>> _byte_convert((1, 1, 0, 1, 0, 1, 1, 0), False, 1)
    214
    >>> _byte_convert((0, 1, 1, 0, 1, 0, 1, 1, 0), True, 1)
    214
    >>> _byte_convert((0, 1, 1, 0, 1, 0, 1, 1, 0), False, 1)
    214
    >>> _byte_convert((86,), True, 8)
    86
    >>> _byte_convert((255, 86), True, 8)
    -170
    >>> _byte_convert((1, 0, 1, 0, 1, 0, 1, 0), True, 1)
    -86
    >>> _byte_convert((2, 2, 2, 2), True, 2)
    -86
    >>> _byte_convert((170,), True, 8)
    -86
    """
    mask = (1 << size) - 1
    number = 0
    for num in data:
        number = (number << size) | num & mask

    if signed and next(iter(data), 0) & 1 << (size - 1):
        return number - (1 << len(data) * size)
    else:
        return number

I said that we can move the onebit2integer check out of the function for bits2integer, and we can. This is simply by passing [onebit2integer(i) for i in iteratebytes(data)] rather than list(iterateints(data)). And now both functions function the same way. So the functions bits2integer and bytes2integer can become:

def bits2integer(data, signed=False):
    r"""
    Converts a b-string into an integer. Both b'0' and b'\x00' are considered zero, and both b'1' and b'\x01' are considered one. Set sign to interpret the number as a 2-s complement signed integer. This is reverse to `integer2bits`.

    Examples:

        >>> bits2integer(b"\x01\x00\x00\x01\x01")
        19
        >>> bits2integer(b"10011")
        19
    """
    return _byte_convert([onebit2integer(i) for i in iteratebytes(data)], signed, 1)

def bytes2integer(data, signed=False):
    r"""
    Converts a b-string into an integer. This is reverse to `integer2bytes`.

    Examples:

        >>> bytes2integer(b'\x00\x00\x00\x13')
        19
    """
    return _byte_convert(list(iterateints(data)), signed, 8)

I've not really read the other functions, but they look ok.

I'd like to point out that Python's default style guide, PEP8, says that we should use snake_case for function names, and yours are not. Style guides are mostly there to create consistency, which you have and so you may want to ignore this. One way to change this could be to have two functions, one that is integer2bits which calls integer_to_bits. And have integer2bits warn on usage, after a couple of years you could, somewhat safely, remove the integer2bits function. This is as I use PEP8, and your functions look strange in my code.

My code is also unlikely to be faster than yours, as I didn't aim for speed. This is as you were duplicating code, and I'd remove that before optimizing your code.

\$\endgroup\$
4
  • \$\begingroup\$ Haha just read what WET is, "we enjoy typing" and"waste everyone's time". But seriously, much of what you said does not apply due to reasons uknown to you. Construct works with both bytes and bits strings so at most I could extract common code into a third function, which aint worth it. Returning lists or generators, forget it, this breaks API. You reinvented your own API. Docstrings are NOT doctests, test suite is somewhere else. And I do not respect PEP8 except few things they got right. \$\endgroup\$
    – ArekBulski
    Commented Oct 11, 2016 at 21:06
  • \$\begingroup\$ EDITED question, tests linked. \$\endgroup\$
    – ArekBulski
    Commented Oct 11, 2016 at 21:08
  • \$\begingroup\$ @ArekBulski Why post on Code Review if you leave "reasons u[n]known to you" out, and don't want someone to review your code? I returned a single generator, in a private function, which you'd know was private if you'd read the second line of my answer, or knew PEP8. What do you want me to do about your tests, "waste everyone's time" even more? \$\endgroup\$
    – Peilonrayz
    Commented Oct 11, 2016 at 23:13
  • \$\begingroup\$ Sorry, didnt mean to be rude. Just that often enough I had to do with people that instead of suggesting improvements, they write elaborate manuscripts where they literally change everything, that by the way, also break everything else. Review is nice. This site is called Code Review. Rewriting everything is not a "review". Not that I dont appreciate the effort, just try to suggest an improvement not a redesign. Do you see what my objection is? If I may shed some light on how the rest of the library looks like, just ask me. \$\endgroup\$
    – ArekBulski
    Commented Oct 12, 2016 at 2:03

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.