3

I've been tasked with going through some old code at work to clean it up and I stumbled across a class:

public class XMLLayer : Object, IDisposable

First I find that the explicit Object inheritance is unnecessary, but then I find that the dispose method is rather useless:

public void Dispose()
{
    Dispose();
}

Nowhere is an instance of XMLLayer in a using block, having its Dispose() explicitly called, or being placed in a IDisposable variable (polymorphism).

Am I wrong in assuming that the idea of the Dispose method was to add your own custom cleanup code for the class?

Corey Ogburn
  • 24,072
  • 31
  • 113
  • 188
  • Are you saying that the `Dispose` method calls itself? – Gabe May 18 '11 at 18:30
  • 3
    Woa, calling Dispose there is probably going to result in a stack overflow... – Etienne de Martel May 18 '11 at 18:30
  • 4
    I think the original developer thought `Dispose` meant "dispose of the process". You know, as in, send it to binary hell. – BoltClock May 18 '11 at 18:31
  • Your assumption is correct about running custom cleanup code for an object in Dispose. You don't need to call Dispose() on an object if it is instantiated with using statement. It is done for you automatically when the object goes out of the using statement scope. – Sergey Akopov May 18 '11 at 18:32
  • 1
    @Gabe, @Etienne de Martel, actually, it would. This is the original code (cut and paste) so that would be a problem. My best guess is that he meant base.Dispose() (which does not exist). There might be a reason he doesn't work here any more... – Corey Ogburn May 18 '11 at 18:34

4 Answers4

3

To answer your question:

Am I wrong in assuming that the idea of the Dispose method was to add your own custom cleanup code for the class?

See the following question and its accepted answer:

Proper use of the IDisposable interface

Community
  • 1
  • 1
Kev
  • 118,037
  • 53
  • 300
  • 385
1

That method isn't just useless - it'll cause a StackOverflowException and terminate the app if it's ever called (or just hang that thread forever, if the JIT has made used tail recursion).

Is there anything in XMLLayer which looks like it really needs to be disposed? When C# was new, some people decided to always implement IDisposable "just in case". Fortunately that doesn't happen much these days, as far as I can see.

You should implement IDisposable if it are directly or indirectly holds an unmanaged resource - e.g. if there's a field of type Stream.

Jon Skeet
  • 1,421,763
  • 867
  • 9,128
  • 9,194
  • Not to me. There's a dozen strings, another class (LayerInfo) with the same IDisposable issue (including EXACT same endlessly recursive Dispose function), and a List. – Corey Ogburn May 18 '11 at 18:36
  • @Corey: Sounds like now is the time to remove the Dispose method and the whole of `: Object, IDisposable` :) – Jon Skeet May 18 '11 at 18:39
1

That dispose doesn't seem so much useless as it is dangerous. it will call itself until eventually it kills the application with a StackOverflowException

You are correct that Dispose is for cleanup. But it's mainly for cleanup of unmanaged resources. Those are the resources that that .NET can't even know exists and therefore won't cleanup either

AGuyCalledGerald
  • 7,882
  • 17
  • 73
  • 120
Jan-Peter Vos
  • 3,157
  • 1
  • 18
  • 21
0

No you're right. If that Dispose() method were ever called, it would just call itself repeatedly forever anyway, which is a pretty good indication it's never being used.

Tim Destan
  • 2,028
  • 12
  • 16