8

Background:

Similar to this question, I am looking to use IDisposable for something other than what it was designed for.

The goal:

The application for this isn't terribly relevant, but just for a good example: I have a Windows Forms application where I implement an Undo design pattern. Typically that is done by intercepting "Value Changed" sorts of events from UI elements. For a DataGridView, CellEndEdit and so forth. However there are cases in which I programmatically change data, and I want to do the things each Undo action tracks, without tracking them.

So far:

I have a way to do all that, but I manage my "Should I Undo" logic on a count:

private int _undoOverrides = 0;
public bool ShouldUndo { get { return _undoOverrides < 1; } }
public void DoNotUndo() { ++_undoOverrides; }
public void ResumeUndo() { --_undoOverrides; }

Now, this works well but you have to remember to call ResumeUndo() at the end of such business logic that begins with DoNotUndo(). And I thought:

Maybe I'm not too much of an idiot to screw that up, but what if I had to expose this interface in code I pass down? I would like if the compiler could take care of this for me, if possible.

The idea:

I am considering using a class implementing IDisposable for this. That way, a user of my code could use a using block and never worry about housekeeping chores. This is what I have so far:

private static int _refCount = 0;
public static int ReferenceCount { get { return _refCount; } }
class HallPass : IDisposable
{
    protected bool bActive;
    public HallPass()
    {
        ++Program._refCount;
        bActive = true;
        Console.WriteLine("Acquired hallpass!");
    }
    public void Dispose()
    {
        if (bActive)
            --Program._refCount;
        bActive = false;
        Console.WriteLine("Hallpass expired!");
    }
}

I included a boolean so that I'm sure I don't double-count back down on Dispose(). So all you have to do to use a HallPass is:

using (new HallPass())
{
    // do things...
}

The question is:

Is this a good idea? Why might this be a bad idea? Any gotchas I should know about?

Also, and I feel stupid for this, but I'm pretty sure reference count is not the right term for it. It's like a reference count, but there's no reference to manage or memory to free. Edit: It could be, it's just not right now.

It's like a mutex or a critical section in that you're trying to make an exception (another misnomer, because I don't mean the kind you throw) to a rule during a section of code, but it's not either of those because they're meant to be mutually exclusive within a scope--this is designed to be done in a nested fashion if you wish. That's why it's a count, not a boolean.

Community
  • 1
  • 1
Gutblender
  • 1,340
  • 1
  • 12
  • 25
  • The static variable puts me off. If I did do something like this I would have the first "BeginUndo" create an appropriate thread-local context which would then be used in any nested "BeginUndo's". Like a TransactionScope, I would require an explicit "Commit" operation.. and then do be aware that this begins to sound like [STM](http://en.wikipedia.org/wiki/Software_transactional_memory) which is *not a trivial problem*. (In the past with a similar approach I've let each disposable/nested-context register an "on-commit/on-revert", but that can get complicated in a hurry..) – user2864740 Sep 13 '14 at 21:09
  • Same here about the static variable--I just whipped this up in a console app to ilustrate the idea. – Gutblender Sep 13 '14 at 21:11
  • 1
    Except for the static variable, this construct is perfectly fine, and a much better approach than the previous one. I tend to call this a *scope* object. It's used in many places (TransactionScope, many methods in Rx and so on) – Lucas Trzesniewski Sep 13 '14 at 21:36
  • @Gutblender The major change I would consider is switching to maybe a static queue which to me seems the appropriate data structure for implementing `undo`-like features. Using `Dispose` seems kinda strange, however I don't see any problems except maybe violating some design principles, for this you may take a look at `C# destructors`. Also, have you considered to use an abstract class since it seems that you need the same logic everywhere, so you can as well d write it only once? – Leron Sep 13 '14 at 21:50
  • Instead of a static variable, create another class to hold the refcount. Pass an instance of that class to all `HallPass` instances that are supposed to participate in the same ref count. – usr Sep 13 '14 at 22:30

1 Answers1

2

The first concern of mine is Program._refCount can be accessed from more than one threads and it is not being synchronized. But you can argue that your application is single threaded.

The next and bigger concern is you are not really using the disposable pattern in the way it is supposed to be used. I personally believe that using a pattern in the way it should be used is important, especially when you are not the only person who is working on the code.

  1. Instead of remembering to call ResumeUndo(), now you need to keep in mind that you must call Dispose(). Question is: will it be natural for your team members to realize that they need to call Dispose () when they want to use the HallPass? (using statement is nice, but it cannot be used in every scenario. If there is a HallPass who lives longer than the scope of a single method, you cannot wrap it in a using statement)
  2. Although it is bad to forget call Dispose() on IDisposible, it usually does not affect the correctness of your program - yes, your program will have performance problems, leaks etc., but functionally it usually is still correct. However for your HallPass, if anyone forgets to call Dispose(), I suppose there will be a functional bug. And the bug is hard to trace.
Xinchao
  • 2,929
  • 1
  • 24
  • 39
  • Good thoughts. I apparently misunderstood some points of using `IDisposable`s. I thought `Dispose()` was called when it went out of scope. Now that I know [that's not true](http://stackoverflow.com/questions/1832992/calling-dispose-vs-when-an-object-goes-out-scope-method-finishes), I wonder how I ever thought that. – Gutblender Sep 13 '14 at 23:25
  • The purpose of `IDisposable` is that if there is an operation that needs to get done, the code which creates the need for that operation doesn't have to remember what it's called. Using a static counter seems like a bad idea; I'd much rather see a factory method which constructs a `HallPass` that contains a count, but using reference counts in conjunction with `IDisposable` is sometimes an excellent idea for solving what can otherwise be intractable ownership problems. – supercat Sep 15 '14 at 13:42