MemoryQueueBufferStream

This stream class is designed to hold data only until it is read from the stream. As opposed to the MemoryStream which holds data until the stream is closed. It was written some time ago and was based on a sample found online. I can no longer find the source, but I believe I used it as a start and cleaned it up.

I am interested in reviewing the performance both in terms of speed and memory usage (there are a number of spots that create byte arrays and copy data from one array to another).

```
///
/// This stream maintains data only until the data is read, then it is purged from the stream.
///
public class MemoryQueueBufferStream : Stream, IDisposable
{
//Maintains the streams data. The Queue object provides an easy and efficient way to add and remove data
//Each item in the queue represents each write to the stream. Every call to write translates to an item in the queue
private Queue lstBuffers_m;

//If the item on the top of the queue is read, but the amount read is less then the size of the data in the queue,
//the remaining data is put in this buffer because the item from the queue is removed. Subsequent read calls will first read
//from this buffer until it is depleted, then the queue will be read again.
private byte[] binLeftOverFromFirstBuffer_m = null;

private bool bIsOpen_m;

public MemoryQueueBufferStream()
{
this.bIsOpen_m = true;
this.lstBuffers_m = new Queue();
}

///
/// Reads up to count bytes from the stream, and removes the read data from the stream.
///
///
///
///
///
public override int Read(byte[] buffer, int offset, int count)
{
this.ValidateBufferArgs(buffer, offset, count);
if (!this.bIsOpen_m)
{
throw new ObjectDisposedException(null, this.GetType().Name + " is closed");
}

int iRemainingBytesToRead = count;

//If there was data left over return it now before reading fro

Some quick shots

-
ValidateBufferArgs() is not needed, because the Buffer.BlockCopy() method will handle these cases in the same manner. In addition, why do you call the ValidtaeBufferArgs() method before you check for if (!bIsOpen_m) inside the Write() method ?

-
the naming of your member variables is well strange. First you are using hungarian notation which is an anti pattern if it is used this way. There are ways how this could be acceptable but not like this

private bool bIsOpen_m; indicating by the b prefix that it is a bool.

Next, you postfix the member variables with a _m and you always use this. with them. If you pre- or postfix the variables with either m_ or _m than there is no need to use this..

EDIT:

if you e.g prefix an int variable like

int iLength = 1;

and you later decide that a long would be the better fit, you for sure will change the type, but the possibility exists (at a high level) that you miss to change the variables name too and you would have

long iLength =1;

in your code which is just lying.

Please read also: what-is-the-benefit-of-not-using-hungarian-notation

From the first answer


Because its orginal intention (see http://www.joelonsoftware.com/articles/Wrong.html and http://fplanque.net/Blog/devblog/2005/05/11/hungarian_notation_on_steroids) has been misunderstood and it has been (ab)used to help people remember what type a variable is when the language they use is not statically typed. In any statically typed language you do not need the added ballast of prefixes to tell you what type a variable is. In many untyped script languages it can help, but it has often been abused to the point of becoming totally unwieldy. Unfortunately, instead of going back to the original intent of Hungarian notation, people have just made it into one of those "evil" things you should avoid.

From the second answer


It uselessly repeats information with no benefit and additional maintenance overhead. What happens when you change your int to a different type like long, now you have to search and replace your entire code base to rename all the variables or they are now semantically wrong which is worse than if you hadn't duplicated the type in the name.

A fine read about hungarian notation (how it can be done right) is in the first answer: http://www.joelonsoftware.com/articles/Wrong.html

-
you have many places where it is obvious what type you are assigning by checking the right side of the assignment. In such cases you should use the var type and let the compiler determine the correct type.

byte[] newBuffer = new byte[count];

would e.g become

var newBuffer = new byte[count];

-
IDisposable

Please read: proper-use-of-the-idisposable-interface


If the user calls Dispose() on your object, then everything has been cleaned up. Later on, when the garbage collector comes along and calls Finalize, it will then call Dispose again.


Not only is this wasteful, but if your object has junk references to objects you already disposed of from the last call to Dispose(), you'll try to dispose them again!

-
Regions

Please read are-regions-an-antipattern-or-code-smell


Is there a good use for regions?


No. There was a legacy use: generated code. Still, code generation
tools just have to use partial classes instead. If C# has regions
support, it's mostly because this legacy use, and because now that too
many people used regions in their code, it would be impossible to
remove them without breaking existent codebases.


Think about it as about goto. The fact that the language or the IDE
supports a feature doesn't mean that it should be used daily. StyleCop
SA1124 rule is clear: you should not use regions. Never.

StackExchange Code Review Q#93154, answer score: 5