Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

This code is part of the Construct library. Docstrings explain what the code is supposed to do.

@singleton
class VarInt(Construct):
    r"""
    Varint encoded integer. Each 7 bits of the number are encoded in one byte in the stream, having leftmost bit not set when byte is terminal.

    Scheme defined at Google's site:
    https://developers.google.com/protocol-buffers/docs/encoding
    https://techoverflow.net/blog/2013/01/25/efficiently-encoding-variable-length-integers-in-cc/

    Example::

        >>> VarInt.build(16)
        b'\x10'
        >>> VarInt.parse(_)
        16
        >>> VarInt.build(2**100)
        b'\x80\x80\x80\x80\x80\x80\x80\x80\x80\x80\x80\x80\x80\x80\x04'
        >>> VarInt.parse(_)
        1267650600228229401496703205376
    """
    def _parse(self, stream, context, path):
        acc = []
        while True:
            b = byte2int(_read_stream(stream, 1))
            acc.append(b & 0b01111111)
            if not b & 0b10000000:
                break
        num = 0
        for b in reversed(acc):
            num = (num << 7) | b
        return num
    def _build(self, obj, stream, context, path):
        if obj < 0:
            raise ValueError("varint cannot build from negative number")
        while obj > 0b01111111:
            _write_stream(stream, 1, int2byte(0b10000000 | (obj & 0b01111111)))
            obj >>= 7
        _write_stream(stream, 1, int2byte(obj))

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

share|improve this question
4  
Please could you write a summary of what this code is meant to do – Tolani Jaiye-Tikolo Oct 9 '16 at 22:42
    
@TolaniJaiye-Tikolo The title says it all. varint is a well-known protocol. – vnp Oct 9 '16 at 23:08
3  
For the sake of others that are reviewing your code, you should explain in detail what your code does and what you want from the review i.e performance. Please visit the help guide on How to Ask . I would also assume this code is in python , this should have been added in your tags as well. – Tolani Jaiye-Tikolo Oct 9 '16 at 23:22
    
I would take any kind of review, performance or correctness or otherwise. And VarInt is an an encoding, not a protocol, I think. – ArekBulski Oct 10 '16 at 12:32
  • A class with no state is a good indication that the class is not needed. Free functions will do just fine.

  • context and path play no role neither in parsing nor in building. Consider not to pass them.

  • I don't see a need for two passes in parse. Consider

    def _parse(stream):
        num = 0
        while True:
            b = byte2int(_read_stream(stream, 1))
            num << 7
            num |= b & 0b01111111
            if not b & 0b10000000:
                break
        return num
    
  • Instead of spelling out both 0b01111111 and 0b10000000 I recommend to spell just one, and make second its negation. I may recommend using familiar 0x7f and 0x80 hex masks instead (but this is a matter of taste).

share|improve this answer
    
The class is made a singleton by a decorator, and _parse _build method signatures must match rest of the code. Reimplementing _parse looked interesting but it is wrong. Least significant bytes are first in the stream, while your code assumes otherwise. VarInt is little-endian in some sense. – ArekBulski Oct 10 '16 at 12:33

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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