Using Timer class properly

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.TimeBetweenChec开发者_JS百科ksInMiliseconds;
        _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:

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

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.

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

1. 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();}

2. 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.

3. Don't return the underlying timer, instead expose a public methods Start(), Stop(), UpdateInterval(int interval).

4. 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.

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);