Using Timer class properly

Question

I would like to know if the code I wrote is a properly written one, it does function, but I've done bad designs before, so I need to know if I thought this in a proper way.

Code is about using a System.Timers.Timer to do a repeated action each X hours. I read some threads about this subject on stackoverflow and then I tried writing my own class. Here is what I wrote:

namespace MyTool
{
public  class UpdaterTimer : Timer
{
    private static UpdaterTimer _timer;
    protected UpdaterTimer()
    { }

    public static UpdaterTimer GetTimer()
    {
        if (_timer == null)
            _timer = new UpdaterTimer();

        SetTimer(_timer);
        return _timer;
    }

    private static void SetTimer(UpdaterTimer _timer)
    {
        _timer.AutoReset = true;
        _timer.Interval = Utils.TimeBetweenChecksInMiliseconds;
        _timer.Elapsed += new ElapsedEventHandler(_timer_Elapsed);
        _timer.Start();
        DoStuff();
    }

    static void _timer_Elapsed(object sender, ElapsedEventArgs e)
    {
        DoStuff();
    }

    private static void DoStuff()
    {
       //does stuff on each elapsed event occurrence
    }
}
}

Short description:

tried using a singleton pattern since I only need one timer to work
I'm not sure about calling DoStuff() inside the SetTimer() method, it seems redundant. But the logic is that, when the app starts, DoStuff() must run, and then it must run again on each Timer.Elapsed event.

My questions are:

Would you have written this behavior in a different way, given the specification?
Is it ok to use a singleton in this case, or it doesn't make sense?

"to do a repeated action each X hours" that so sounds like a scheduled task to me... — Marc Gravell
– Marc Gravell, Commented Jul 18, 2011 at 20:11
@Tocco thank you for your comment, will remove it from there in this case. — Amc_rtty
– Amc_rtty, Commented Jul 18, 2011 at 20:36
@Andrei, Errr ... I was wrong, DoStuff() will not be called on Start(). Sorry ... =/ — Tocco
– Tocco, Commented Jul 18, 2011 at 20:47
@Marc Gravell - I am concerned about your comment, you mean Timer should not be used at all for scheduled tasks? could you explain why?; @Tocco - no problem — Amc_rtty
– Amc_rtty, Commented Jul 18, 2011 at 20:52
@Andrei no, I don't mean "not at all"; but something on that interval might be easier to schedule as a scheduled task. You don't have to - I'm not forcing you to. I'm just suggesting it to ensure you have considered all the available options. — Marc Gravell
– Marc Gravell, Commented Jul 18, 2011 at 20:56

Noctis · Accepted Answer · 2015-02-04 22:42:24Z

4

I'd venture that making a subclass of Timer is of very little utility. You've turned about 7 lines of code into a whole load of bloat. Given that your DoStuff method is private, this isn't going to be reusable in any way. As such, consider the following few lines:

Action doStuff=()=>{
    //does stuff on each elapsed event occurance
};
_timer=new System.Timers.Timer(){
    AutoReset = true,
    Interval = Utils.TimeBetweenChecksInMiliseconds
};
_timer.Elapsed += (s,e)=>doStuff();
_timer.Start();
doStuff();

where _timer is a property of the containing class. The intent here is clear. I don't think it deserves a class of its own.

Edit:

(s,e)=>doStuff()

is a lambda expression that defines a delegate that takes two parameters s and e (sender and event). As this is added to _timer.Elapsed event (of type ElapsedEventHandler), the compiler can infer the types s and e from the event type ElapsedEventHandler. The only action that our delegate performs is doStuff().

In longhand, this could be expanded to:

_timer.Elapsed += delegate(object sender, ElapsedEventArgs e){doStuff();};

The lambda allows us to write the above considerably more succinctly.

edited Feb 4, 2015 at 22:42

Noctis

11.8k3 gold badges46 silver badges86 bronze badges

answered Jul 18, 2011 at 20:29

spender

121k36 gold badges245 silver badges369 bronze badges

Sign up to request clarification or add additional context in comments.

4 Comments

Tocco Over a year ago

I'd remove the last line doStuff();

spender Over a year ago

Faithful to original design. Perhaps OP wants an immediate invocation prior to waiting for 1st timer tick?

Amc_rtty Over a year ago

thank you very much; as this is my 1st task of this kind, with timed actions, your comment is allowing me to learn; however I cant understand this line, could you please explain it: _timer.Elapsed += (s,e)=>doStuff();

spender Over a year ago

@Andrei : I added some info for you.

Noctis · Accepted Answer · 2015-02-04 22:40:58Z

I don't know what you are trying to achieve but here is a few point about your current design:

Your GetTimer is broken in multithreading mode:

if (_timer == null) _timer = new UpdaterTimer();

suppose you have two threads each one call GetTimer at the same time, the first one checks the _timer and found it null so it proceed. but at that time and before it reach _timer = new UpdateTimer() the thread context switching switch to the other thread and pause the current thread execution. so the other thread check the _timer and find it not null so it proceed and create a new timer, now the context switch rescheduled the first thread and its continue its execution and so create a new timer and update the old one. to correct use of singleton pattern use static constructor instead static UpdaterTimer() { _timer = new UpdaterTimer();}
The developer can call GetTimer() as much as he wanted so it will call _timer.Elapsed += new ElapsedEventHandler(_timer_Elapsed); again registering another Elapsed handler. Also note that whenever you call GetTimer, the timer will start "_timer.Start()" even if it were stopped.
Don't return the underlying timer, instead expose a public methods Start(), Stop(), UpdateInterval(int interval).
At SetTimer() you want to call DoStuff immediately, but that then will be blocked at the SetTimer method waiting DoStuff() to complete, a better way is to start that method in a new thread ThreadPool.QueueUserWorkItem(new WaitCallback((_) => DoStuff())); or use System.Threading.Timer instead of System.Timers.Timer and set it call the method start immediately.

Dmitry · Accepted Answer · 2011-07-19 06:34:44Z

2

I think you should use composition here instead of inheritance. (i.e. do not inherit from Timer, just use a Timer as a private field)
Also if code outside of you class shouldn't change timer's properties, you shouldn't show it to other classes. If you want to give an ability to outer code to subscribe to timer's Elapsed event, you can add an event for it and trigger it in your DoStuff handler, if not -- just hide it.

code expample as requested by OP in comments:
NOTE: It doesn't match specification (it's a whole different usecase, for your usecase you do not need a separate class after all (refer to another answer)) but shows how can you use composition and hide implementation details. (Also the example has some thread-safety issues but that's not the point.)

public static class TimerHelper
{
    private static Timer _timer;

    static TimerHelper()
    {
        _timer = new Timer(){AutoReset=true, Interval=1000};
        _timer.Start();
    }

    public static event ElapsedEventHandler Elapsed
    {
        add { _timer.Elapsed += value; } 
        remove { _timer.Elapsed -= value; }
    }
}

And to use:

// somewhere else in code
TimerHelper.Elapsed += new ElapsedEventHandler(TimerHelper_Elapsed);

edited Jul 19, 2011 at 6:34

answered Jul 18, 2011 at 20:23

Dmitry

3,1271 gold badge20 silver badges26 bronze badges

2 Comments

Amc_rtty Over a year ago

Thank you for your comment; I inherited from Timer so that the UpdateTimer object would have the properties of a Timer. I could not call _timer.Start() so I figured I should inherit Timer. Also, could you please provide a short pseudocode, so I can understand how to "hide" the timer. Sorry for missing what may be obvious.

Dmitry Over a year ago

@Andrei, "hide" means that you declare that fields as private and do not expose (return) its value by any public method or property. Or expose it's individual members. You clearly do not need to inherit from timer because you don't override or add to its behaviour. After some thought, I think I agree with @spender. You just need to create a timer as a field in your application or main window class and do not define a separate class. Still I'll update answer with some code.

Collectives™ on Stack Overflow

Using Timer class properly

3 Answers 3

Edit:

4 Comments

Comments

2 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

Edit:

4 Comments

Comments

2 Comments

Your Answer

Sign up or log in

Post as a guest

Related