Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Can someone review my timer class that runs some code every second, please? The void it's calling (runCode) is just a void with some code in it.

using Reality.Game.Sessions;
using System;
using System.Threading;

namespace Reality.Game.Misc
{
    public static class UserTimer
    {
        public static void Initialize()
        {
                /* Lets rev this bad boy up... */
            Timer timer = new Timer(callback, "Some state", TimeSpan.FromSeconds(1), TimeSpan.FromSeconds(1));
        }

        private static void callback(object state)
        {
            foreach (Session s in SessionManager.Sessions.Values)
            {
                if (s != null && s.Authenticated && !s.Stopped)
                {
                    workTimer(s);
                }
            }
        }

        private static void workTimer(Session s)
        {
            JobCacheWorker.runCode(s);
        }
    }
}
share|improve this question
    
Hi! I see you've added the code, but your question is still pretty unclear. To make it easier for reviewers, please add some context to your question. What does the code do? Why are you doing it? etc. –  RubberDuck Feb 16 at 17:34

1 Answer 1

Timers are pretty simple to use. There are only a few things to worry about and they are called out in the documentation. From that page:

"As long as you are using a Timer, you must keep a reference to it. As with any managed object, a Timer is subject to garbage collection when there are no references to it. The fact that a Timer is still active does not prevent it from being collected."

In your code, the reference to the Timer isn't stored anywhere. This means it's eligible for garbage collection, and when that happens it will stop firing events. So you need to store a reference somewhere, in your code it looks like a static field on the UserTimer class would make sense.

The other thing is if it's possible for the callback to run longer than the interval, you might want to detect this case and have the second callback not do any work. If this is a possibility I'll have a line of code like:

if (!Monitor.TryEnter(timerLock))
    return;

Of course you need to release the monitor in a finally block in your callback.

share|improve this answer

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.