14

I have tried to pass stream as an argument but I am not sure which way is "the best" so would like to hear your opinion / suggestions to my code sample

I personally prefer Option 3, but I have never seen it done this way anywhere else.

Option 1 is good for small streams (and streams with a known size)

Option 2_1 and 2_2 would always leave the "Hander" in doubt of who has the responsibility for disposing / closing.

public interface ISomeStreamHandler
{
    // Option 1
    void HandleStream(byte[] streamBytes);

    // Option 2
    void HandleStream(Stream stream);

    // Option 3
    void HandleStream(Func<Stream> openStream);
}

public interface IStreamProducer
{
    Stream GetStream();
}

public class SomeTestClass
{
    private readonly ISomeStreamHandler _streamHandler;
    private readonly IStreamProducer _streamProducer;

    public SomeTestClass(ISomeStreamHandler streamHandler, IStreamProducer streamProducer)
    {
        _streamHandler = streamHandler;
        _streamProducer = streamProducer;
    }

    public void DoOption1()
    {
        var buffer = new byte[16 * 1024];
        using (var input = _streamProducer.GetStream())
        {
            using (var ms = new MemoryStream())
            {
                int read;
                while ((read = input.Read(buffer, 0, buffer.Length)) > 0) 
                {
                    ms.Write(buffer, 0, read);
                }
                _streamHandler.HandleStream(ms.ToArray());
            }
        }
    }

    public void DoOption2_1()
    {
        _streamHandler.HandleStream(_streamProducer.GetStream());
    }

    public void DoOption2_2()
    {
        using (var stream = _streamProducer.GetStream())
        {
            _streamHandler.HandleStream(stream);    
        }
    }

    public void DoOption3()
    {
        _streamHandler.HandleStream(_streamProducer.GetStream);
    }
}
Adam Plocher
  • 13,994
  • 6
  • 46
  • 79
Jakob Dyrby
  • 152
  • 1
  • 1
  • 7

2 Answers2

3

Option 2_2 is the standard way of dealing with disposable resources.

Your SomeTestClass instance asks the producer for a stream - then SomeTestClass owns a stream and is responsible for cleaning up.

Options 3 and 2_1 rely on a different object to clean up the resource owned by SomeTestClass - this expectation might not be met.

Option 1 is jut copying a stream's content to another stream - I don't see any benefits in doing that.

dcastro
  • 66,540
  • 21
  • 145
  • 155
  • Option 3 just uses "MemoryStream" as a means to be able to do convert the stream to a byte array. - So it is not a matter of parsing stream content to another stream. – Jakob Dyrby Dec 11 '13 at 14:12
  • So to be clear; if you where asked to impliment "void HandleStream(Stream stream);" you would assume that your implementation had no responsibility to close / dispose the stream? – Jakob Dyrby Dec 11 '13 at 14:15
  • @JakobDyrby - Still, Option 3 is reading from a `Stream`, writing to a `MemoryStream`, and converting to a `byte[]`. Why not just read from the `Stream` and use the buffer then? – dcastro Dec 11 '13 at 14:15
  • @JakobDyrby: Yes. If a disposable resource (not just a stream) is passed as an argument to my method, I wouldn't assume I should close it. In fact, I would say it's my responsibility **not** to close it. I don't know who's calling me, I don't know if whoever is calling me will need further access to that stream - so it's my obligation to leave it open. – dcastro Dec 11 '13 at 14:17
  • @JakobDyrby As a rule of thumb, the keyword is _ownership_. Whoever owns the `IDisposable` instance, is responsible for closing it. – dcastro Dec 11 '13 at 14:23
  • In my first comment I meant to say: "Option 1" and not "Option 3" of cause - sorry. – Jakob Dyrby Dec 12 '13 at 08:01
  • Ok so in my case - who actually _owns_ the IDisposable (stream). In other words what defines ownership? – Jakob Dyrby Dec 12 '13 at 08:06
  • I don't want the "SomeTestClass" to own the stream - I just want the class to supply the means to access a stream. The "SomeTestClass" has no use for it. That's why I prefer Option 3. Why shouldn't that be totally fine? – Jakob Dyrby Dec 12 '13 at 08:18
  • let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/43022/discussion-between-jakob-dyrby-and-dcastro) – Jakob Dyrby Dec 12 '13 at 09:04
1

You may not realize it, but you are attempting to implement the pipeline design pattern. As a starting point, consider taking a look at:

With regards to your implementation, I recommend that you go with option #2:

public interface IStreamHandler
{
    void Process(Stream stream);
}

With regards to object lifetime, it is my belief that:

  • the implementation should be consistent in how it handles calling Dispose
  • your solution will be more flexible if IStreamHandler did not call Dispose (now you can chain handlers together much like you would in Unix pipes)

THIRD-PARTY SOLUTIONS

Building a pipeline solution can be fun, but it is also worth noting that there are existing products on the market:

ADDITIONAL NOTES

There is a design issue related to your proposed Option 2:

void Process(Stream stream);

In Unix Pipes you can chain a number of applications together by taking the output of one program and make it the input of another. If you were to build a similar solution using Option 2, you will run into problems if you are using multiple handlers and your data Stream is forward only (i.e. stream.CanSeek=False).

Community
  • 1
  • 1
Pressacco
  • 2,815
  • 2
  • 26
  • 45
  • Thank you! I will read in to that design pattern. But for now; could you please elaborate on who you think has the responsibility to close dispose. Should I use the interface as shown in option 2_1 or in option 2_2? – Jakob Dyrby Dec 11 '13 at 14:23
  • I would go with: DoOption2_2. More importantly: **(1)** be consistent with your implementation, and **(2)** ensure that all resources are release in the event that an exception is thrown unexpectedly. – Pressacco Feb 13 '14 at 21:24