3
\$\begingroup\$

I got tired of starting timer every time I need to do something simple as running some function 5 seconds after something in my code happens. So I tried to write a function (actually, 3 of them: setTimeout, setInterval and clearInterval) to write one line of code when I need it instead of, say, five.

It is working, at least for the cases I tested.

I understand that maybe something in that code is not right, maybe I forgot about something. Should I improve anything in my approach?

using System;
using System.Windows.Forms;

namespace timeouts_test
{
    public partial class Form1 : Form
    {
        private Timer[] timerList = new Timer[100];
        private int timerMaxIndex = -1;
        public Form1()
        {
            InitializeComponent();
            int aInterval = SetInterval(500, a);
            int bInterval = SetInterval(500, b);
            int cInterval = SetInterval(500, c);

            SetTimeout(3500, () =>
            {
                ClearInterval(aInterval);
            });

            SetTimeout(7000, () =>
            {
                ClearInterval(bInterval);
            });

            SetTimeout(10500, () =>
            {
                ClearInterval(cInterval);
            });
        }

        public void a()
        {
            label1.Text += "a";
        }

        public void b()
        {
            label1.Text += "b";
        }

        public void c()
        {
            label1.Text += "c";
        }

        public int SetInterval(int time, Action function)
        {
            Timer tmr = new Timer();
            tmr.Interval = time;
            tmr.Tick += new EventHandler(delegate (object s, EventArgs ev)
            {
                function();
            });
            tmr.Start();

            timerMaxIndex++;
            var index = timerMaxIndex;
            timerList[timerMaxIndex] = tmr;            
            return index;
        }

        public int SetTimeout(int time, Action function)
        {
            Timer tmr = new Timer();
            tmr.Interval = time;
            tmr.Tick += new EventHandler(delegate (object s, EventArgs ev)
            {
                function();
                tmr.Stop();
            });
            tmr.Start();

            timerMaxIndex++;
            var index = timerMaxIndex;
            timerList[timerMaxIndex] = tmr;            
            return index;
        }

        public void ClearInterval(int interval)
        {
            if (timerList[interval] == null)
                return;

            timerList[interval].Stop();
            timerList[interval] = null;
            if (interval == timerMaxIndex)
                timerMaxIndex--;
        }
    }
}

(Functions a, b and c were made just to test this.)

\$\endgroup\$
  • 1
    \$\begingroup\$ What's wrong (IMO) is to...do it. You do not need to (for example) use integer IDs when you have objects. Also you're using System.Windows.Forms.Timer, it's not accurate, it wastes more GDI resources and it won't fire if UI is busy doing something else. Just use .NET timers as they are, you'll write less code and it'll be more robust (it doesn't mean you can't write few helper functions, just don't try to do things in JavaScript style using C#, it's...weird and I would say also wrong). \$\endgroup\$ – Adriano Repetti Dec 11 '15 at 13:32
  • \$\begingroup\$ @AdrianoRepetti about timers, which one should I use? System.Timers.Timer? or something else? \$\endgroup\$ – serge1peshcoff Dec 11 '15 at 13:43
  • \$\begingroup\$ @AdrianoRepetti and which way will be more C#-style? I want to wrap it into helper because I don't want to write repeatable code many times \$\endgroup\$ – serge1peshcoff Dec 11 '15 at 13:45
  • \$\begingroup\$ There are three timers you may use in a WinForms application: System.Windows.Forms.Timer, System.Threading.Timer and System.Timers.Timer. To pick right one refer to MSDN (they have different use cases). Try to use Timer (also the one you're using now) without mimicking JavaScript (also try to drag & drop timer directly from WinForms designer) and you'll see you have less code than what you wrote... \$\endgroup\$ – Adriano Repetti Dec 11 '15 at 13:58
  • \$\begingroup\$ Just as a side note, here's a blog implementing a similar idea: JavaScript-like timers. dailycoding.com/posts/… \$\endgroup\$ – Arturo Torres Sánchez Jan 26 '16 at 2:41
2
\$\begingroup\$

The following is from Wikipedia

Modularity, and hence separation of concerns, is achieved by encapsulating information inside a section of code that has a well-defined interface

That's why we should first think about the primary concerns of our application, and try to seperate these concerns into classes, which can then also be re-used if designed modularity in mind.

Therefore, Assuming what you are trying to achieve is something like this:

ExecutionPlan aInterval = ExecutionPlan.Repeat(500, a);
ExecutionPlan bInterval = ExecutionPlan.Repeat(500, b);
ExecutionPlan cInterval = ExecutionPlan.Repeat(500, c);

ExecutionPlan.Delay(3500, () => { aInterval.Abort(); });
ExecutionPlan.Delay(7000, () => { bInterval.Abort(); });
ExecutionPlan.Delay(10500, () => { cInterval.Abort(); });

You can wrap your timers in ExecutionPlan class.

Using a class for wrapping the timers have many benefits.

  1. You keep track of class instances and not bare integer values,
  2. You can re-use this class in other projects or forms (if designed & implemented correctly),
  3. You can extend this class to enrich it's functions
  4. etc.. (There are an infinite number of benefits)

Here is my ExecutionPlan class which works with an underlying System.Timers.Timer:

public class ExecutionPlan : IDisposable
{
    private System.Timers.Timer planTimer;
    private Action planAction;
    bool isRepeatedPlan;

    private ExecutionPlan(int millisecondsDelay, Action planAction, bool isRepeatedPlan)
    {
        planTimer = new System.Timers.Timer(millisecondsDelay);
        planTimer.Elapsed += GenericTimerCallback;
        planTimer.Enabled = true;

        this.planAction = planAction;
        this.isRepeatedPlan = isRepeatedPlan;
    }

    public static ExecutionPlan Delay(int millisecondsDelay, Action planAction)
    {
        return new ExecutionPlan(millisecondsDelay, planAction, false);
    }

    public static ExecutionPlan Repeat(int millisecondsInterval, Action planAction)
    {
        return new ExecutionPlan(millisecondsInterval, planAction, true);
    }

    private void GenericTimerCallback(object sender, System.Timers.ElapsedEventArgs e)
    {
        planAction();
        if (!isRepeatedPlan)
        {
            Abort();
        }
    }

    public void Abort()
    {
        planTimer.Enabled = false;
        planTimer.Elapsed -= GenericTimerCallback;
    }

    public void Dispose()
    {
        if (planTimer != null)
        {
            Abort();
            planTimer.Dispose();
            planTimer = null;
        }
        else
        {
            throw new ObjectDisposedException(typeof(ExecutionPlan).Name);
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ I notice that in GenericTimerCallback, if a operation is called that does not resolve immediately, the Delay function will repeat itself. Moving the Abort() call above planAction(); seems to correct that. \$\endgroup\$ – Tieson T. Jul 7 '16 at 19:20
  • \$\begingroup\$ It must be noted, that there should be used System.Timers.Timer not the System.Windows.Forms.Timer \$\endgroup\$ – T.Todua Nov 9 '18 at 7:05
3
\$\begingroup\$

Right now your timer methods have dependencies on stuff that shouldn't be.

You're relying on System.Windows.Form. That's blocking you if you ever want to use these methods in a console application or well... stuff that isn't related to WinForms. Check for the System.Timers.Timer class!

Your helper methods are relying on variables that aren't in their scope (The Timer[]). That's a dependency you want to remove.

To do so, we need a way to stop the timer. We could return the timer, but that might be a little too much for what we want to do. So let's create an interface, IInterruptable.

public interface IInterruptable
{
    void Stop();    
}

Now, we need something to wrap our Timer so we can implement the interface.

public class TimerInterrupter : IInterruptable
{
    private readonly Timer _timer;

    public TimerInterrupter(Timer timer)
    {
        if (timer == null) throw new ArgumentNullException(nameof(timer));
        _timer = timer;
    }

    public void Stop()
    {
        _timer.Stop();
    }
}

So far so good. That deals with the problem of the Timer[].

Next, let's see how we can change your implementation using System.Timers.Timer.

Point one, not totally related to the new implementation but very related to your old one. Using delegate is a little old-school. Since C#... 3? There are Functions you can use, that are backward compatible to delegate. That means :

//Syntax might not be good, but you get the point
fooBar.SomeEvent += new delegate(object,object){asd();};

Becomes :

fooBar.SomeEvent += (a,b) => asd();

So, let's look at that timer! This Timer class as an AutoReset property, which well... auto-resets the timer at the end of each interval is set to true. Which will be the main difference between your SetInterval and SetTimeout methods.

public static IInterruptable SetInterval(int interval, Action function)
{
    Action functionCopy = (Action)function.Clone();
    Timer timer = new Timer { Interval = interval, AutoReset = true };
    timer.Elapsed += (sender, e) => functionCopy();
    timer.Start();

    return new TimerInterrupter(timer);
}

Notice that the method is now static, since it's an helper method. You'll be able to use it in any context, which is great!

I changed the time parameter to interval, as it's clearer this way. You didn't pass a time, you passed a time interval as a parameter. It's important to be very clear.

I'm copying the Action. I'm not 100% sure that's useful, but it's to avoid a problem with references. If for example I did :

Action someFunction = CreateAFunctionThatIsntNull();
SetInterval(1,someFunction);
someFunction = null;

You're screwed. So copying the Action has a purpose here.

Now the SetTimeout :

public static IInterruptable SetTimeout(int interval, Action function)
{
    Action functionCopy = (Action)function.Clone();
    Timer timer = new Timer { Interval = interval, AutoReset = false };
    timer.Elapsed += (sender, e) => functionCopy();
    timer.Start();

    return new TimerInterrupter(timer);
}

Notice something? Well, that's the same code, with one different variable in AutoReset. So let's take care of that by extracting another method :

public static class TimerHelper
{
    public static IInterruptable SetInterval(int interval, Action function)
    {
        return StartTimer(interval, function, true);
    }

    public static IInterruptable SetTimeout(int interval, Action function)
    {
        return StartTimer(interval, function, false);
    }

    private static IInterruptable StartTimer(int interval, Action function, bool autoReset)
    {
        Action functionCopy = (Action)function.Clone();
        Timer timer = new Timer { Interval = interval, AutoReset = autoReset };
        timer.Elapsed += (sender, e) => functionCopy();
        timer.Start();

        return new TimerInterrupter(timer);
    }
}

That's the final version in my opinion. I think it looks better. Though there's a final problem. SetTimeout and SetInterval are terrible method names. I know the idea is to copy the Javascript functions, but let's make an exception. These methods names are not intuitive and overall pretty terrible. Maybe something like ExecuteEvery instead of SetInterval and ExecuteIn instead of SetTimeout would be better names.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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