I think you're doing too much in one function, hiding the big picture I'd want to read without going into the implementation details. I'd first start with a broad overview:
using (var reader = new StreamReader(e.GetDataStream().AsStreamForRead())
{
SkipOptionalZlibHeader(reader);
var uncompressedStream = ReadAndDecompressStream(reader);
DecodeJson(uncompressedStream);
}
With:
const int ExpectedZLibHeaderMarker = 0x78;
const int ExpectedZLibHeaderSize = 2;
static void SkipOptionalZlibHeader(BinaryReader reader)
{
if (!HasZlibHeader(reader))
return;
reader.Seek(CalculateZLibHeaderSize(reader), SeekOrigin.Current);
}
static bool HasZlibHeader(BinaryReader reader)
=> reader.PeekChar() == ExpectedZLibHeaderMarker;
static long CalculateZLibHeaderSize(BinaryReader reader)
=> return ExpectedZLibHeaderSize;
Stop here for one second. Here you made some HUGE assumptions:
- Compression flags are always
0x78
. That's not true because if message contract simply says that it's a compressed zlib stream with zlib header then they may change it without breaking the contract. Also note that there is no reason the raw compressed stream cannot start with that magic number. You may want to try to decode the trimmed stream and if it fails then try to decode it as it had no header. Better: be sure it has (or not) the header and stick to it otherwise you're deploying code that may fail. - Header size is always two bytes. According to the flags there might be more than that.
- Everything else in the stream is data. No, there is also a footer with a checksum.
Note that we dropped one useless copy because SkipOptionalZlibHeader()
simply seek after the header. As exercise for the reader: if underlying stream does not support seeking (check CanSeek
property) then you need to read and discard those bytes.
Note that ReadAndDecompressStream()
is returning a Stream
, the fact that you reuse the same MemoryStream
multiple times (very good things if you have a measured and truly beneficial gain in memory footprint and/or performance) is an implementation detail and I'd love to keep its callers unaware of that. Also there is no reason to reuse DeflateStream
. We may then implement it as this:
static Stream ReadAndDecompressStream(BinaryReader reader)
{
// Go back to beginning for writing
_output.Position = 0;
using (var decompressor = new DeflateStream(_output, CompressionMode.Decompress, true))
{
reader.CopyTo(decompressor);
}
// Go back to beginning for reading
_output.Position = 0;
return _output;
}
There is something that really bothers me: we're returning an IDisposable
object (MemoryStream
) but caller does not have its ownership (and then it has not to dispose it). It's not intuitive and it must be documented. Also note that your actual code is working just because an implementation detail: StreamReader
calls Dispose()
on the provided Stream
and your code is working just because disposing MemoryStream
has not visible effects (but it's an implementation detail). We also have to do something for that.
Let's first change ReadAndDecompressStream()
to return a StreamReader
instead and change its name to something more meaningful:
static StreamReader CreateDecompressedReader(BinaryReader reader)
{
// Go back to beginning for writing
_output.Position = 0;
using (var decompressor = new DeflateStream(_output, CompressionMode.Decompress, true))
{
reader.CopyTo(decompressor);
}
// Go back to beginning for reading
_output.Position = 0;
return new StreamReader(_output, Encoding.UTF8, false, _output.Length, true);
}
Now caller has the ownership of the disposable object:
using (var reader = new StreamReader(e.GetDataStream().AsStreamForRead())
{
SkipOptionalZlibHeader(reader);
using (var decompressedReader = CreateDecompressedReader(reader))
DecodeJson(decompressedReader);
}
Note that I removed ?.
, in your original code if e.GetDataStream()?
returned null
then the first call to datastr.CopyTo()
fails. If this may happen then properly handle the case.
As final step you should add some error checking. With I/O things may go wrong then you'd better be prepared (especially if you keep those assumptions in-place), just don't add catch (Exception)
and be specific.