8

I have the following code example:

public interface IRepository {
   // Whatever
}

public class SampleRepository : IRepository {
   // Implements 'Whatever'
}

public class NHibernateRepository : IRepository, IDisposable {

   // ...

   public void Dispose() { ... }
}

Now - is that really bad? I'm not sure, but this seems to be pretty the same as not marking the destructor of the base class virtual in C++.

I don't want to make the IRepository interface implement IDisposable, because that would bring unwanted complexity and bunch of classes which would also have to implement IDisposable.

How should this case be handled?

I'm sure this can happen to any type hierarchy - when one of the derived type has to manage disposable resources.

So what should I do - pull the IDisposable up to the very first interface or leave it as it and hope user would distinguish disposable and non-disposable repositories?

Thank you.

Ken D
  • 5,880
  • 2
  • 36
  • 58
Yippie-Ki-Yay
  • 22,026
  • 26
  • 90
  • 148

6 Answers6

15

Yes, I'd say this is bad - because you can't use all repositories in the same way.

I would personally make the repository interface extend IDisposable - it's easy enough to implement it with a no-op, after all.

This is exactly the same choice as is made by Stream, TextReader and TextWriter in the main framework. StringWriter (for example) doesn't need to dispose of anything... but TextWriter is still disposable.

This all comes about because IDisposable is in some ways an odd interface: it doesn't convey something that is offered to callers... it conveys something which is required of callers (or at least, strongly encouraged with possible issues if you don't go along with it).

Jon Skeet
  • 1,421,763
  • 867
  • 9,128
  • 9,194
  • 1
    +1, `because you can't use all repositories in the same way` and good sample – Saeed Amiri Dec 13 '10 at 17:19
  • However, Stream and TextWriter are classes which don't force inheritors to implement disposal logic. I think an abstract base class is more appropriate here. – Matt Brunell Dec 13 '10 at 18:37
  • @Matt: It may be appropriate to have an abstract base class, yes - *possibly* in addition to the interface. – Jon Skeet Dec 13 '10 at 18:43
9

The only issue you might have is if you are using some type of factory, Inversion of Control, or Dependency Injection pattern / framework. If you ever want to use the object as an interface, you will never be able to do this:

IRepository repo = Factory.GetRepository();
repo.Dispose();

You may want to introduce a INHibernateRepository that implements IDisposable. Because IDisposable is usually such a low level operation, there's not a huge issue with making your interfaces implement this interface.

poindexter12
  • 1,775
  • 1
  • 14
  • 20
4

No, do not "pull the IDisposable up". Keep interfaces atomic, simple and independent.

Unless you could argue that being IDisposable is a fundamental trait of IRepository (and SampleRepository shows it isn't) it there should be no derivation between the two.

H H
  • 263,252
  • 30
  • 330
  • 514
  • If an interface will sometimes be used in conjunction with another, one should define the interfaces separately, and define an interface which inherits both but adds nothing new. Anyone who happens to inherit both should implement the combined one, which will implicitly implement the others. If the vast majority of objects which implement one will implement both, it may not be worthwhile to have a separate interface for the one that's never alone. With IDisposable, which can be trivially implemented by any class, it's easiest to include it always than have two Repository interfaces. – supercat Dec 14 '10 at 00:30
3

That seems perfectly fine to me. What's the problem?

Matt Lacey
  • 65,560
  • 11
  • 91
  • 143
3

What you've described is okay with one important proviso: an object of a type which has added 'IDisposable' to its base must never be passed to a consumer which may end up holding or persisting the object for an unknown duration.

Basically, the owner of an IDisposable object must(*) either take care of disposing the object itself, or hand the object off to a new owner which can accept and honor the responsibility. Initially, IDisposable objects are generally "owned" by their creator, but hand-offs are common. A reference to an IDispoable object can be given to another object without transferring ownership; in that scenario, the owner is still responsible for disposing the object when it is no longer needed. For that to happen, the owner has to know the object is no longer needed. The most common patterns are:

  1. The object is passed as a parameter to a method; the object hosting the method will not use the object after the method returns.
  2. The object is passed as a parameter to a method which will store it in some object's field, but that object can later somehow be requested to destroy the reference.
  3. The disposable is passed as a parameter to a method which is hosted by an object to which the owner of the disposable object holds all references; the hosting object won't use the disposable object unless requested to, and the owner of the disposable object will know if it'll never issue any more such requests.

If one of those patterns applies, you may be in good shape. If not you may be in trouble.

supercat
  • 77,689
  • 9
  • 166
  • 211
  • would it be considered a good solution to implement a factory around the creation of those IDisposable instances when the consumer "is not sure" how/when to dispose and let the factory keep the track of instance and then dispose them when needed? – dexter Dec 13 '10 at 20:10
  • The $50,000 question is whether it the creator/owner of the object will know when it's no longer needed, and can call Dispose on it then. If a user of the object might store a reference someplace the owner doesn't know about, and if something else might attempt to use the object at some unknown future time, it may be impossible to dispose the item cleanly. But if you know when the item will no longer be needed, there's no problem. – supercat Dec 13 '10 at 23:34
2

First, to answer your question. The dispose pattern is different from a C++ destructor. A Dispose method is intended to dispose of resources contained by the class, instead of disposing of the class itself.

The reason for marking C++ destructors as virtual does not exist in .NET because every instance of a reference type has a sync block which contains run time type information. Using this, the garbage collector can appropriately reclaim the correct amount of memory.

As for extending IRepository with IDisposable, that would be a quick fix which would be acceptable in the vast majority of cases. The only objection that I can see is that extending the interface will require all derived classes to implement the interface. On the face, it may seem easy to implement an interface with NOPs (multiple times perhaps), but you shouldn't have to. But, I can offer an alternative.

Instead, consider using an abstract base class which implements the Dispose Pattern. This would follow the structure of the "standard" implementation of the dispose pattern.

public abstract class Repository : IDisposable  
{ 
    public void Dispose() { Dispose(true); }
    protected virtual Dispose(bool disposing) {  } 
}

public class NHibernateRepository : Repository { /* Impl here, add disposal. */ }

public class TestRepository : Repository { /* Impl with no resources */ }

Take a look at the Dispose Pattern Guidelines written by Microsoft to see a more detailed example.

Matt Brunell
  • 10,141
  • 3
  • 34
  • 46
  • If you're going to leave out the finalizer part of the IDisposable pattern, there's no point in using the secondary Dispose that takes a boolean flag - just make Dispose() public virtual... – SoftMemes Dec 13 '10 at 20:36
  • I shortened the example to illustrate using a base class, instead of extending an interface. But, even so, I believe there is value in following the standard pattern. First, it is more easily recognized. Second, inheritors can choose include finalization. – Matt Brunell Dec 13 '10 at 20:50