Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

I used this code in several projects. But I am not sure if it is stable. However on production it is working well, but I want to listen your advices:

/// <summary>
/// Standard abstract parent for Timer-powered classes. Has Start|Stop|Pulse events
/// </summary>
public abstract class TimerContainer : IDisposable
{
    protected Timer Timer { get; set; }
    protected TimeSpan Interval { get; set; }

    protected TimerContainer() : this(TimeSpan.FromSeconds(1)) { }

    protected TimerContainer(TimeSpan interval)
    {
        Status = TimerStatus.NotInitialized;
        Timer = new Timer(OnTick, null, Timeout.Infinite, Timeout.Infinite);
        Interval = interval;
    }

    protected abstract void OnTick(object sender);

    #region Standard Events
    public event EventHandler OnStart;
    public event EventHandler OnStop;
    public event EventHandler OnPulse;

    public TimerStatus Status { get; protected set; }

    public void Start()
    {
        Timer.Change(TimeSpan.Zero, Interval);
        Status = TimerStatus.Running;

        if (OnStart != null) {
            OnStart(this, new EventArgs());
        }
    }

    public void Stop()
    {
        Status = TimerStatus.Stopped;
        Timer.Change(Timeout.Infinite, Timeout.Infinite);

        if (OnStop != null) {
            OnStop(this, new EventArgs());
        }
    }

    protected void InvokePulse()
    {
        if (OnPulse != null) {
            OnPulse(this, new EventArgs());
        }
    }

    #region IDisposable Members

    protected bool disposed = false;
    ~TimerContainer()
    {
        Dispose(false);
    }
    public virtual void Dispose()
    {
        if (!disposed) {
            Dispose(true);
            GC.SuppressFinalize(this);
            disposed = true;
        }
    }
    protected virtual void Dispose(bool disposing)
    {
        if (disposing) {
            if (Status != TimerStatus.Stopped) {
                Stop();
            }

            if (Timer != null) {
                Timer.Dispose();
                Timer = null;
            }
        }
    }

    #endregion
    #endregion
}

/// <summary>
/// Timer status
/// </summary>
public enum TimerStatus
{
    NotInitialized = 0,
    Running = 1,
    Stopped = 2
}
share|improve this question
Hi Orif, i loved your code and i wonder, for what the use of OnPulse ? should the derived class put the InvokePulse be in OnTick ? – guyl Apr 11 '12 at 13:00

1 Answer

up vote 1 down vote accepted

One thing is that the way you're calling the events is not thread-safe:

if (OnStop != null) {
    OnStop(this, new EventArgs());
}

If the only subscriber to the event unsubscribes between the null check and the invoke, you will get a NullReferenceException. The correct way is like this:

var onStop = OnStop;
if (onStop != null) {
    onStop(this, new EventArgs());
}

Or, alternatively, you can make sure that the events are never null by adding a subscriber that does nothing:

public event EventHandler OnStart = delegate { };

That way, you don't have to check for null at all. This could hurt performance a bit, but it shouldn't be noticeable (especially since the events most likely won't be invoked more frequently than once every few milliseconds).

Another thing is that your finalizer is completely useless, since you don't have any unmanaged resources. It could make sense if some of the children of this class did have some unmanaged resources, but I think those could have their own fianlizer.

Also, in the usual Dispose() pattern, the parameterless Dispose() method is not virtual. I don't see any reason for it here, having virtual Dispose(bool) should be enough.

share|improve this answer
Tips about null check and virtual Dispose() is great. But I am not sure on removing Dispose. Anyway, I marked your post as an answer. Thanks! – Orif Apr 10 '12 at 14:28
I didn't say anything about removing Dispose(). I meant the finalizer: ~TimerContainer(). If that's what you meant, why aren't you sure about it? – svick Apr 10 '12 at 14:43

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.