Implementing a very basic IDisposable

Question

I would like to implement a very simple IDisposable.
The whole idea is to measure the execution time of my methods which they all return a MethodResult. e.g.

public class MethodResult : IDisposable
{
    private Stopwatch _StopWatch;
    public MethodResult()
    {
        _StopWatch = new Stopwatch();
        _StopWatch.Start();
    }
    public object Result { get; set; }
    public TimeSpan ExecutionTime { get; set; }

    public void Dispose()
    {
        _StopWatch.Stop();
        ExecutionTime = _StopWatch.Elapsed;
    }
}

Usage:

static MethodResult TestMehodResult()
{
    using (var result = new MethodResult())
    {
        result.Result = 666;
        Thread.Sleep(1000);
        return result;
    }
}

My question is really simple: is implementing only Dispose() method is sufficient for this case, or should I implement the entire Dispose pattern in my class?
There are no resources to free in my class.

Bonus question: Is there a better pattern to measure execution time of a method instead of using an IDisposable like I did?

Sorry if this question is dumb. I'm really new to .net
Thanks in advance.

No, it is not a good way. It will function as you want, but it will be confusing for everyone. Disposing an object is supposed to render it useless, but you are "disposing" it right before returning. Compare this to [another abuse of IDisposable](https://stackoverflow.com/q/2101524/11683) which feels much more justified. — GSerg, Oct 13 '19 at 07:06
The longer form (standard Microsoft) pattern has two main advantages. a) It handles unmanaged resources. b) It handles inheritance. You don't need either, so your form is fine for your purposes. _@GSerg has rightly questioned whether you should use `Dispose` - my comments are purely comparing your code to the longer form - I am not saying you should or shouldn't use `Dispose`._ — mjwills, Oct 13 '19 at 07:07
@mjwills The class is [not sealed](https://stackoverflow.com/a/898867/11683), so a longer form is still required. — GSerg, Oct 13 '19 at 07:08
Rather than `return result;` consider `return result.SomeProperty;` where `SomeProperty` provides the necessary data. This allows you to use the `Dispose` pattern, but deals with @GSerg's primary (and valid) concern. — mjwills, Oct 13 '19 at 07:09
`so a longer form is still required.` In the theoretical sense, yes. In the practical sense, no. But sure, adding `sealed` to the class would be clearer. — mjwills, Oct 13 '19 at 07:09
@mjwills, Sorry for being stupid but I really don't understand what @GSerg is saying. `"Disposing an object is supposed to render it useless, but you are "disposing" it right before returning"`. And I need to result result becoause it holds the entire class I need for my methods (it also contains other properties such as `Error` object etc...) — zig, Oct 13 '19 at 07:13
It is odd (not illegal, but odd / non-standard) to have an object where you continue to use it after being disposed. But that is exactly what you are doing - your `TestMehodResult` method `Dispose`s it, then returns it. It will _work_, but it is odd. The solution is to separate the current class into two classes. The new class will have the `Result` and `ExecutionTime` properties. Then use `return result.YourNewPropertyWhichPopulatesYourNewClass;` rather than `return result;`. — mjwills, Oct 13 '19 at 07:17
@mjwills, Ahhh. thanks. now I see what you are both saying. never thought of that. I only did this because it looked like a "nice", fast and readable way to handle my (already existing many many) methods. I suppose I could use a `try`/`finally result.Finalize()` before returning but that looks kinda ugly to me. I also did not understand the issue with the `sealed` keyword. in practice `MethodResult ` is a base class for descendants (`MethodResult `) so I guess it can not be sealed? — zig, Oct 13 '19 at 07:35
@mjwills, You mentioned that in the longer form `It handles inheritance.` I just tested with a descendant class where `public class MethodResult2 : MethodResult` changing the result of my method to return `MethodResult2` worked just fine. the `Dispose` method was invoked. so I don't see an issue with inheritance here. — zig, Oct 13 '19 at 07:44
Though the way of using disposables like this is quite unusual, it wouldn't be a problem (I also created a similar [Profiler](https://github.com/koszeggy/KGySoft.CoreLibraries#performance-measurement) once). What is confusing here is this `MethodResult` class, which is actually a measurement scope rather than a result of a method. What if the method is void? Or if not a single method is measured? I called it `MeasureOperation` but it is not exposed as a public type. Feel free to use or investigate the source under the link. — György Kőszeg, Oct 13 '19 at 08:11
@GyörgyKőszeg, Thanks. I see what you mean. in *my case* there are no void methods. they are actually all web service methods for which *always* return `MethodResult`. I would like the response (result) to contain `ExecutionTime` (but I guess that is irrelevant to the actual question?). — zig, Oct 13 '19 at 08:38

score 3 · Accepted Answer · answered Oct 13 '19 at 08:53

3

To stay true to the concept of scope, you can inject your result in the IDisposable's constructor. Using an interface to keep things flexible. I'm surprised nobody mentioned the loss of type safety in your method, I'd definitely add the generic type parameter to the base MethodResult class (as you mention it to be in the comments).

public interface ITimed
{
    TimeSpan ExecutionTime { get; set; }
}

public class MethodResult<T> : ITimed
{
    public T Result { get; set; }
    public TimeSpan ExecutionTime { get; set; }
}

public class MethodTimer : IDisposable
{
    private readonly Stopwatch _StopWatch;
    private ITimed _result;

    public MethodTimer(ITimed result)
    {
        _result = result;
        _StopWatch = new Stopwatch();
        _StopWatch.Start();
    }

    public void Dispose()
    {
        _StopWatch.Stop();
        _result.ExecutionTime = _StopWatch.Elapsed;
        _result = null;
    }
}

Usage

static MethodResult<int> TestMehodResult()
{
    var timedResult = new MethodResult<int>();

    using (var timer = new MethodTimer(timedResult))
    {
        timedResult.Result = 666;
        Thread.Sleep(1000);
    }

    return timedResult;
}

answered Oct 13 '19 at 08:53

Funk

10,976
1
17
33

1

Thanks. this is basically what I started doing after reading @GyörgyKőszeg comment. why the `_result = null` in the `MethodTimer.Dispose()` method needed? wouldn't it set the result to null? – zig Oct 13 '19 at 09:11
2

@zig The result will outlive the timer, so we dispose (no pun intended) of its reference to mark the timer for garbage collection as soon as possible. – Funk Oct 13 '19 at 09:15
Setting it to null isn’t necessary and has no benefit here. If you use the using pattern, the `timer` instance will go out of scope at the same time Dispose is called, so the GC already knows that `timer` can be freed and doesn’t hold a reference to `_result` anymore. – ckuri Oct 13 '19 at 09:21
`Setting it to null isn’t necessary and has no benefit here.` It has benefit if `MethodTimer` has a longish lifetime (e.g. not scoped within a function). I agree with @Funk's implementation here - there is no _downside_ of setting it to `null`. It often won't help, but it _sometimes_ will. – mjwills Oct 13 '19 at 10:26

score 2 · Answer 2 · answered Oct 13 '19 at 09:12

2

Yes, that's fine, but I'd probably advise "sealing" the class; there can be no question over whether it needs a more complex virtual Dispose(bool) API, finalizer support, if you simply declare it as:

public sealed class MethodResult : IDisposable

because now:

it can't be subclassed, so you know you don't need to deal with polymorphism
it doesn't have a finalizer, and you know that a subclass doesn't

So: very explicit and obvious.

answered Oct 13 '19 at 09:12

Marc Gravell

1,026,079
266
2,566
2,900

1

But what if it needs to be sub-classed but has no resources to free? in my test I see that the Dispose() is being invoked just fine in sub-classed descendant. – zig Oct 13 '19 at 09:16
3

@zig if it can be subclassed **by external types**, then the base class **doesn't know** that there are no resources - managed or unmanaged - to free (it shouldn't know much about the derived classes). In that case, I **would** use the `protected virtual Dispose(bool disposing)` approach. If it can only be subclassed by things *in your control* (i.e. it is an `internal` type or has only `internal` constructors): then sure, you can cheat and use the extra knowledge that you have – Marc Gravell Oct 13 '19 at 09:31

Implementing a very basic IDisposable

2 Answers2