-4

I'm refactoring some code, unleashing Resharper on it, and came across this:

public virtual void Dispose()
{
    this.Dispose();
}

...which R# flags as potentially problematic with "Function is recursive on all paths"

Which makes sense; however, the "official" (straight from the horse's mouth) code is somewhat similar (Dispose calls Dispose):

public void Dispose()
{
    Dispose(true);
    // This object will be cleaned up by the Dispose method. 
    // Therefore, you should call GC.SupressFinalize to 
    // take this object off the finalization queue 
    // and prevent finalization code for this object 
    // from executing a second time.
    GC.SuppressFinalize(this);
}

...that code wont' even compile, though; I get, "No overload for method 'Dispose' takes '1' arguments"

So how can I both implement Dispose() and not make it recursive?

UPDATE

If I try this (from here):

try
{
    Dispose(true); //true: safe to free managed resources
}
finally
{
    base.Dispose();
}

...I get, "'object' does not contain a definition for 'Dispose'" and "No overload for method 'Dispose' takes '1' arguments"

Community
  • 1
  • 1
B. Clay Shannon-B. Crow Raven
  • 8,547
  • 144
  • 472
  • 862
  • 7
    The two are clearly not the same. The MSDN example calls a different `Dispose` method from inside the outer `Dispose`. – Daniel Kelley Jun 18 '14 at 16:01
  • 1
    See http://stackoverflow.com/questions/538060/proper-use-of-the-idisposable-interface – ken2k Jun 18 '14 at 16:03
  • So what should I do in my Dispose method (which I must have, as the class extends IDisposable)? I shouldn't call Dispose() because it's recursive, so what should I call i is leaving it blank a good option? Or calling some base method? – B. Clay Shannon-B. Crow Raven Jun 18 '14 at 16:03
  • 2
    No one can sensibly tell you what to call if we can't see what you actually need to dispose in the first place. The pattern is relatively clear, so I'm not sure what about it you are not sure about. – Daniel Kelley Jun 18 '14 at 16:04
  • 1
    Well why are you implementing IDisposable at all? Presumably you have some resources to dispose. So dispose of them. – Jon Skeet Jun 18 '14 at 16:04
  • 1
    If you implement the interface it must be because you have something to dispose in your class. If not, dont implement it... – Bun Jun 18 '14 at 16:06
  • RE: Update - Put the keyboard down and step away. – Keith Payne Jun 18 '14 at 16:38

1 Answers1

2

Looks like you need to implement the Dispose Pattern correctly:

public class MyThingWithResources : IDisposable
{
    ~MyTHingWithResources()
    {
        // This is a finalizer and is an optional part.
        // Finalizers are not generally required, and are 
        // intended for clearing up unmanaged resources
        // that your class might hold

        // Finalizers are called by the garbage collector
        // So you can never be sure when they are going to
        // be called.

        this.Dispose(false);
    }

    public void Dispose()
    {
        this.Dispose(true);
        GC.SuppressFinalize(this);
    }

    protected virtual void Dispose(bool disposing)
    {
        if (disposing)
        {
            // if disposing is true, then the method 
            // is being called as part of a managed disposal
            // this means it should be safe to dispose of managed 
            // resources (.Net classes such as System.IO.Stream )
        }

        // If disposing is false, the Dispose method was called 
        // from the finaliser, so you're in the last chance saloon 
        // as far as tidying stuff up is concerned.
        // Only unmanaged resources (and the objects that wrap them)
        // should be tidied up in this scenario
    }
}
RikRak
  • 898
  • 1
  • 7
  • 21
  • 1
    How do you know `IDisposable` is needed given we have no idea what they want to dispose? Also, finalizers/destructors are generally only needed when dealing with unmanaged resources. – Daniel Kelley Jun 18 '14 at 16:20
  • That Dispose() is shown in my question; it won't compile for me. – B. Clay Shannon-B. Crow Raven Jun 18 '14 at 16:22
  • 1
    @B.ClayShannon, that Dispose() is *not* shown in your question. Honest question -- do you understand (a) the code you're viewing or (b) what you're trying to accomplish (and why)? It's not obvious that you do, and we can't help you if you don't have some minimal understanding of the actual problem. – Anthony Pegram Jun 18 '14 at 16:35
  • I'm simply trying to refactor the code; how it works I'm not sure. I didn't write it, and it is extremely obscure. – B. Clay Shannon-B. Crow Raven Jun 18 '14 at 16:52