HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Packing and unpacking bits

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
andbitsunpackingpacking

Problem

Part 2 here.

I wrote this class for packing data.

Would it benefit from being named BitPacker rather than BitStream (and change write/read to pack/unpack), or it works as is?

How can I improve it? Any way to make it more efficient? How can I reduce the amount of code? I have a lot of copy/pasted code with minor things changed that I'm not sure how (if?) I can condense.

And, of course, any other comments.

```
using System;
using System.Collections;
using System.Collections.Generic;

///
/// Used to store data as bits. Acts as a queue - first data added is the first data removed.
/// Data should be read in the same order it is written. If read in a different order, it gives undefined results.
/// Reading from an empty BitStream returns 0.
///
public class BitStream
{
ulong scratch_write;
int scratch_write_bits;
ulong scratch_read;
int scratch_read_bits;
Queue buffer;

///
/// How many bits are currently in the BitStream
///
public long StoredBits
{
get;
private set;
}

#region Constructors
///
/// Make a new BitStream
///
public BitStream()
{
scratch_write = 0;
scratch_write_bits = 0;
scratch_read = 0;
scratch_read_bits = 0;
buffer = new Queue();
}

///
/// Make a new BitStream
///
/// How many bits you expect this stream will hold. A closer value nets increased performance.
public BitStream( long bitCount )
{
scratch_write = 0;
scratch_write_bits = 0;
scratch_read = 0;
scratch_read_bits = 0;
buffer = new Queue( (int) IntDivideRoundUp( bitCount, 64 ) );
}

///
/// Make a new BitStream containing bits from the byte array
/// NOTE: StoredBits may return a higher count than there are actual bits to read if the byte array came from another BitStream.
///
/// contains bits to be stored in the bitstream
public BitStream( byte[] bits )
{

Solution

Bug

The swap() method isn't doing what you think it does, because the passed in T obj1, T obj2 aren't passed with the ref keyword the values ar only changed in that method not targeting the variable values of the calling method.

So this

int min = 10;
int max = 5;
swap(min, max);
Console.WriteLine(String.Format("{0} : {1}", min, max);


will output 10 : 5

But using the ref keyword wouldn't be that good either.

-
using regions is considered to be a antipattern, not only because of the reasons statet in the answer of the link, but also because having 4 regions indicates that your class is doing too much.

-
based on the .NET naming guidelines variables should be named using camelCase instead of snake_case casing.

-
you should always use braces {} although they might be optional. Using them will make your code less error prone.

The least you should do is that you stick to a choosen style. Right now you are mixing the styles, sometimes using braces and sometimes you don't , e.g

if ( data2 < 0 )
{
    data2 = ~data2 | ( 1L << ( bits - 1 ) );
}


-
by using constructor chaining, you can remove some of the duplicated code like so

public BitStream()
{
    scratch_write = 0;
    scratch_write_bits = 0;
    scratch_read = 0;
    scratch_read_bits = 0;
    buffer = new Queue();
}

public BitStream(long bitCount)
    : this()
{
    buffer = new Queue((int)IntDivideRoundUp(bitCount, 64));
}

public BitStream(byte[] bits)
    : this()
{
    foreach (var bite in bits)
    {
        Write(bite, byte.MinValue, byte.MaxValue);
    }
}


or you can just initialize some of the values like so

ulong scratch_write = 0;
int scratch_write_bits = 0;
ulong scratch_read = 0;
int scratch_read_bits = 0;
Queue buffer = new Queue();
public BitStream()
{ }

public BitStream(long bitCount)
{
    buffer = new Queue((int)IntDivideRoundUp(bitCount, 64));
}

public BitStream(byte[] bits)
{
    foreach (var bite in bits)
    {
        Write(bite, byte.MinValue, byte.MaxValue);
    }
}


-
the spaces after opening ( and before the closing ) are looking strange in my eyes. A C# developer wouldn't expect them.

protected int BitsRequired( long min, long max )

Code Snippets

int min = 10;
int max = 5;
swap(min, max);
Console.WriteLine(String.Format("{0} : {1}", min, max);
if ( data2 < 0 )
{
    data2 = ~data2 | ( 1L << ( bits - 1 ) );
}
public BitStream()
{
    scratch_write = 0;
    scratch_write_bits = 0;
    scratch_read = 0;
    scratch_read_bits = 0;
    buffer = new Queue<ulong>();
}

public BitStream(long bitCount)
    : this()
{
    buffer = new Queue<ulong>((int)IntDivideRoundUp(bitCount, 64));
}

public BitStream(byte[] bits)
    : this()
{
    foreach (var bite in bits)
    {
        Write(bite, byte.MinValue, byte.MaxValue);
    }
}
ulong scratch_write = 0;
int scratch_write_bits = 0;
ulong scratch_read = 0;
int scratch_read_bits = 0;
Queue<ulong> buffer = new Queue<ulong>();
public BitStream()
{ }

public BitStream(long bitCount)
{
    buffer = new Queue<ulong>((int)IntDivideRoundUp(bitCount, 64));
}

public BitStream(byte[] bits)
{
    foreach (var bite in bits)
    {
        Write(bite, byte.MinValue, byte.MaxValue);
    }
}
protected int BitsRequired( long min, long max )

Context

StackExchange Code Review Q#134028, answer score: 8

Revisions (0)

No revisions yet.