Is this a valid object disposal code pattern in method with return?

Question

I noticed the following object disposal code pattern in a C# project and I was wondering if it's acceptable (although it works).

public object GetData()
{
    object obj;

    try
    {
        obj = new Object();

        // code to populate SortedList

        return obj;
    }
    catch
    {
        return null;
    }
    finally
    {
        if (obj != null)
        {
            obj.Dispose();
            obj = null;
        }
    }
}

For this example, I'm using a general 'object' instead of the actual IDisposable class in the project.

I know that the 'finally' block will be executed every time, even when the value is returned, but would it affect the return value (or would it be a new object instance) in any way since the object is being set to null (for what seems like object disposal and GC purposes).

Update 1:

I tried the following snippet and the return object is non-null, although the local object is set to null, so it works, which is a bit strange considering some of the comments below:

public StringBuilder TestDate()
{
    StringBuilder sb;

    try
    {
        sb = new StringBuilder();

        sb.Append(DateTime.UtcNow.ToString());

        return sb;
    }
    catch
    {
        return null;
    }
    finally
    {
        sb = null;
    }
}

Btw, I'm using C# 4.0.

P.S. I'm just reviewing this project code. I'm not the original author.

Update 2:

Found the answer to this mystery [1]. The finally statement is executed, but the return value isn't affected (if set/reset in the finally block).

[1] What really happens in a try { return x; } finally { x = null; } statement?

Answer 1

This code will compile fine (assuming that you are not actually using an Object but something that implements IDisposable), but it probably won't do what you want it to do. In C#, you don't get a new object without a new; this code will return a reference to an object that has already been disposed, and depending on the object and what Dispose() actually does, trying to use a disposed object may or may not crash your program.

I assume the idea is to create an object, do some stuff with it, then return the object if successful or null (and dispose the object) on failure. If so, what you should do is:

try {
    obj = new MyClass();
    // ... do some stuff with obj
    return obj;
}
catch {
    if(obj != null) obj.Dispose();
    return null;
}

Answer 2

Simply using the using statement achieves the same result as that, and is the standard practice

public int A()
{
    using(IDisposable obj = new MyClass())
    {
        //...
        return something;
    }
}

I would, however, advise against returning your IDisposable object. When you dispose of an object, it is supposed to be considered "unusable". And so, why return it?

If the object's lifetime needs to be longer than the method A's lifetime, consider having the calling method B instantiate the object, and pass it as a parameter to method A. In this case, method Bwould be the one using the using statement, inside which it would call A.

Answer 3

If you are returning an IDisposable object, then it is the responsibility of your caller to dispose of it:

public IDisposable MakeDisposableObject()
{
    return new SqlConnection(""); // or whatever
}

caller:

using (var obj = MakeDisposableObject())
{
}

It makes less than no sense for your method to dispose of an object and then return it. The disposed object will be of no value to the caller. In general, referencing a disposable object which has been disposed should produce an ObjectDisposedException.

Answer 4

A few observations.

That code wouldn't compile because object doesn't have a .Dispose() method.

Why wouldn't you use IDisposable?

Why would you dispose of an object that is being returned, since returning you would return an object for the purpose of some other code to use it. The concept of "disposing" of something is to give it a chance to clean up after itself and its used, un-managed resources. If you are returning an object that is supposed to be used elsewhere, but has unmanaged resources that you want to clean up before the object gets used anywhere else, then you shuld really have 2 separate objects. One to load some data that would be disposable, and another object that would contain the usable loaded content that you want to pass around. An example of this would be something like stream readers in the .NET framework. You would normally new a stream reader, read it into a byte[] or some other data object, .Dispose() the stream reader, then return the byte[]. The "loader" that has some resources to dispose of in a timely fashion is separate from the object containing the "loaded" data that can be used without needing to be disposed.