3

If MyClass implements IDisposable interface, will the both given snippets disposes MyClass object correctly? and which method is preferable over the other?

Code 1

public MyModel MethodOne()
{
    using (MyClass myClass = new MyClass())
    {
        var myModel = new MyModel();
        myModel.MyModelData = myClass.GetData();

        return myModel;
    }
}

Code 2

public MyModel MethodTwo()
{
    var myData = new MyData();

    using (MyClass myClass = new MyClass())
    {
        myData = myClass.GetData();
    }

    var myModel = new MyModel();
    myModel.MyModelData = myData;

    return myModel;
}
Improve this question
4
  • 3
    you are not using MyClass into those using Commented Dec 4, 2013 at 11:28
  • @giammin Sorry... Fixed the code... Commented Dec 4, 2013 at 11:31
  • If MyClass is only used in the GetData Context and nowhere else then i would prefer 2. Commented Dec 4, 2013 at 11:32
  • 3
    The 2nd snippet doesn't score any points for constructing a needless instance of MyData(). With code like this, you'd normally ought to worry about exactly what's going to happen with the myData object when the object that created it gets disposed. If that also makes myData invalid then Bad Things happen. Commented Dec 4, 2013 at 11:52

4 Answers 4

Reset to default
3

It is the same: the object will be disposed in both way.

the using statement is translated by the compiler as a try block which calls Dispose in a finally block.

finally is guaranteed to execute after the try block has finished execution, regardless of its execution path.

Dispose is guaranteed to be called, no matter what.

Reference MSDN

As a style and best practice IMHO it is better to dispose an object as soon as it is not needed anymore because you will free resources earlier and because if Dispose throw an exception you will have your method return anything. and it will be easier to debug.

Update

As KrisVandermotten pointed out there are some cases where the finally block will not execute.

  • Environment.FailFast
  • the Uncatchable execeptions
  • Power Failure
Improve this answer
Sign up to request clarification or add additional context in comments.

10 Comments

I often read that a finally block is guaranteed to execute, "no matter what". This is obviously not true. It will not execute when you have a power failure before it has a chance to execute. There are more interesting cases though, such as out of memory conditions and forced process termination.
Mind the "forced termination". I've seen it many times when dealing with unmanaged library that simply 'terminated' the process with a 'accessviolation'-like crash, completely not minding the managed layer of the application.. Oh, I just rememered: it was not access-violation, but stack-corruption. Nastier.
@Nalaka526 maybe it will never be disposed and resources will not be free until you close the application. It is highly reccomended to dispose those objects. it is not only a matter of memory, think about file access
@Nalaka526 Do not confuse disposing of unmanaged resources with freeing memory. In general, unless the unmanaged resource being freed happens to be unmanaged memory, no memory is freed by calling Dispose(). Certainly, calling Dispose() on an object will not cause that object's memory to be released.
I find it simplest to say that a using block essentially guarantees that the system will do the Dispose before it tries to do anything in an outer execution context; Dispose will be prevented by the same things that would prevent reaching an outer execution context.
|
3

using is translated to try with finally block. Your code

using (MyClass myClass= new MyClass())
{
    myData = myClass.GetData();
}

Is the same as

MyClass myClass= new MyClass();
try
{
    myData = myClass.GetData();
}
finally
{
    if (myClass != null)
        ((IDisposable)myClass).Dispose();
}

As for which method is preferable, both solutions will work fine. The second one disposes of the object earlier and for that reason is preferable, in many cases. There's no need to hold on to objects, especially if they use external resources.

There are exceptions to that, though. You may want to or need to hold on to an object for longer to achieve your objective, e.g. when using a transaction scope. Also, sometimes the objects derived from the disposable object (as in your case) may become invalid if the originating object is disposed.

Improve this answer

3 Comments

Almost. Remember that IDisposable might be implemented explicitly. The real code is documented at msdn.microsoft.com/en-us/library/yh598w02.aspx.
"The second one disposes of the object earlier and for that reason is preferable" - not always. Imagine usig RAII-based approach to "UnitOfWork" or other "Transaction"-like things. You actually usually want to have such using at the topmost level to ensure that any unexpected exceptions perform a rollback. But that of course strictly depends on what you want. Hence, there is actually no "universally preferable" way. Btw, see HansPassant's comment under the question.
@quetzalcoatl You're right. Eh, I should avoid making general rules... Added those comments to my answer to make it better. Thanks.
2

In both cases, you can rely on the fact that, if an object of MyClass was constructed and the method completes, the object is also disposed.

In the latter case, the object is constructed later than in the first case, and it is disposed earlier than in the first case, but in both cases it will be disposed.

Which is better? Assuming that a disposable object somehow holds on to something that needs to be freed, and that this holding on to it is expensive, all else being equal, the better approach would be the second.

So why the two caveats above?

"if an object of MyClass was constructed"

In the second case, it is possible that the call to new MyData() throws an exception, and the MyClass object construction is not even attempted. In the first case, constructing this object is the first thing that happens. It may fail, but it will be tried. It may fail in the second case as well of course. And if it fails, there is nothing to be disposed.

"and the method completes"

Nothing will be disposed if you lose power before the dispose can be executed, and there are a few other pathological cases where execution of a finally block is not guaranteed.

Improve this answer

3 Comments

Yes, but the new MyData(); call in the second was somewhat arbitrary because that value is never used - myData is reassigned inside the using block - so the first useful thing both functions do is to construct the MyClass instance.
@Damien_The_Unbeliever True, but the point is that an exception may be thrown before the object is constructed, because some code runs. What code it actually is that runs and potentially causes this exception to be thrown is irrelevant for the argument.
But so far as I can see, this new extra code was only added because the OP seems to have gone var-blind and couldn't work out how to declare myData outside of the using block without also assigning it a value.
1

IMHO the first version is more concise and therefore better, since the extra time taken before disposing is likely to be negligible. It typically doesn't take long to construct a couple of objects and set a property.

Only if there were a call to "myModel.DoSomethingThaTakesALongTime()" would I consider using the second version.

Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.