Overengineering IOpenable

published: Tue, 12-Apr-2005   |   updated: Thu, 27-Oct-2005
Eurydice, number two cat

A week or so ago, I published an article about an IOpenable interface and OpenableBase class I'd been developing. I went ahead and posted the article despite a few misgivings: I felt as if I were missing something important about the code I was posting. I worked on the code a little bit afterwards but was unable to find out what was wrong. It took Mike Scott, an old friend from yesteryear, to point out that I seemed to be making it more difficult than it should be. He was quite gentle about it for a Scotsman, but in reality he should have beat me about the head with a haggis. Yes, gentle reader, I've been guilty of over-engineering.

Mike's point was definitely valid. He said, essentially, that why wasn't this code my scenario:

using (MyFile f = new MyFile("test.del")) {
  f.Open();
  // do work with the opened f; it will be closed automatically
}

You know those times when you look at some code and you suddenly realize what a dipstick you've been? Well, looking at Mike's code scenario was one of those. My head hit the desk with a thud. Methinks I owe Mike a McEwans or something.

Without further ado, here's the new code for the IOpenable/OpenableBase combo:

  interface IOpenable : IDisposable {
    void Open();
    void Close();
  }

  class OpenableBase : IOpenable {
    private bool isOpen;
    private bool isDisposed;

    ~OpenableBase() {
      Dispose(false);
    }

    public void Open() {
      if ((!isDisposed) && (!isOpen)) {
        internalOpen();
        isOpen = true;
      }
    }

    public void Close() {
      Dispose(true);
    }

    public bool IsOpen {
      get { return isOpen; }
    }

    private void Dispose(bool disposing) {
      if (!isDisposed) {
        if (isOpen) {
          internalClose(!disposing);
          isOpen = false;
          if (disposing)
            GC.SuppressFinalize(this);
        }
        isDisposed = true;
      }
    }

    public void Dispose() {
      Dispose(true);
    }

    protected virtual void internalOpen() {
    }

    protected virtual void internalClose(bool inFinalizer) {
    }
  }

The only extra thing I can think of at the moment is to throw an exception if Open() happened to be called on a disposed instance. I think that's a justifiable contract between the code and its user, don't you? I don't think it's worth throwing an exception if you call Open() on an already opened instance, nor do I think it's worth the notification that you happen to be calling Dispose()/Close() on an already-disposed instance.

But Open() on a disposed instance is enforcement of the contract that the instance can only be opened and closed once. Maybe I'll add it later, for now I'm going to cook this haggis that hit me on the head.