patterncsharpMinor
Packing and unpacking bits
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 )
{
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
So this
will output
But using the
-
using
-
based on the .NET naming guidelines variables should be named using
-
you should always use braces
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
-
by using constructor chaining, you can remove some of the duplicated code like so
or you can just initialize some of the values like so
-
the spaces after opening
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.