I wrote some code today and It was changed by another developer who said it was more safe. I am not sure this is right as I cannot see the advantage of what was done here are some code examples

public byte[] ReadFile(Stream stream)
{
    byte[] result = null;

    try
    {
       // do something with stream
       result = <result of operation>
    }
    finally
    {
        stream.Close();
    }

    return result;
}

this was changed to

public byte[] ReadFile(Stream stream)
{
    byte[] result = null;

    // do something with stream
    result = <result of operation>

    return result;
}

I am quite new to c# should the stream not be closed when you are finished with it ?

• c#
• exception-handling
• io

In general the first version would be a bad design.

Yes, the Stream should be closed but preferably by the same code (same method) that opened it. That is called separation of concerns and it makes mistakes and confusion much less likely.

So either

• your ReadFile() accepts for example a string fileName and Opens and Closes the Stream, or
• you leave it to the caller.

Your method (the 2nd version) should be used in this manner:

using (var reader = new FileReader(...))
{
  // maybe some pre-reading
  var r = ReadFile(reader);
  // maybe some post-reading
}

Note that the 2nd approach also makes the method more reusable.

There is no right answer on this question, cause it depends on the your app architecture.

I would say, yes, cause if in this function stream is not created, but only used, so closing of it let's let up to the caller.

But I repeat, it depends on your app architecture.

Who opens the door must remember to close it.

So it's better to close the stream in the method who has opened it.

Typically the creator of the Stream should be the one to close it, ideally by disposing it with a using block:

using (var myStream = getMeAStream()) {
    ReadFile(myStream);

    // If you want to be really sure it is closed:
    myStream.Close();
    // Probably not neccessary though, since all 
    // implementations of Stream should Close upon Disposal
}

The stream has been passed into this function by something else, you are not responsible in the function to close it , it is up to the calling code to deal with that issue.

The reason is that another operation (like reset or another read) may need to be done externally to this function and if you close it you will cause an exception.

I used to refactor code like this several times a day until finally someone listened to me.

I have seen at least one serious bug caused by this sort of code, so its a bad idea in general

The code review was right - never do such a thing (or if something like that is required by an outside code then that code is probably not designed properly - there are exceptions but for things like streams disposing there almost never is, and some default 'patterns' are always to be followed).
in most cases use streams like this (from 'outside')...

using(Stream stream = new ...)
{
    ...call your method
}

(or e.g. reader is disposing of it - but it's recommended that you do something like this in either case - equivalent is using a 'finally' block, but boils down to the same thing)

...basically the calling function would never know if you disposed of it 'inside' or not - until it crashes out. If 'both parts' agree on something like that, well it's still not acceptable but could go unpunished.

It's not simply a C# problem. After the call of function "ReadFile" you need to use the stream for other operations? If no you could choose to close the stream after you read the file. It's better closing streams when you finish to use them, but generally I prefer to close them in the same context than opens them, because it's there the place where you know if you need stream for other operation:

Stream s = open_the_stream()
try {
  ReadFile(s)...
} finally {
  s.Close();
}

In any way, close stream when you terminate to use them.