Implementing a Scheduler(something that implements a given schedule)

Question

I am trying to implement a class that implements a given schedule. I am trying to design this Scheduler class so it can be paused and everything. I have just written some example code so I can better convey my requirements. I would be grateful if more experienced minds could take a look and tell me if what I have right now is the right way to go about things or if there are better ways to do this. I'd really appreciate some thoughts.

class Scheduler {
 private object locker = new object();
 private System.Timers.Timer timer = new System.Timers.Timer();
 private int[] schedule = null;
 private int scheduleIndex = 0;
 private bool offline = true;
 private bool timerElapsed;
 private Thread th;
 public Scheduler() {
 timer.Elapsed += new System.Timers.ElapsedEventHandler(TimerElapsedHandler);
 th = new Thread(RunSchedule);
 th.Start();
 }
 public void RunSchedule() {
 lock (locker) {
 while (schedule == null || scheduleIndex > schedule.length) // this will not throw a NullReferenceException since schedule == null is being checked first
 Monitor.Wait(locker);
 while (offline)
 Monitor.Wait(locker);
 // pick up next schedule and start timer and wait till timer finishes
 int nextSchedule = schedule[scheduleIndex];
 // call external program to execute schedule
 timer.Interval = 120 * 1000;
 timerElapsed = false;
 timer.Start();
 while(!timerElapsed)
 Monitor.Wait(locker);
 }
 }
 public void Offline() {
 lock (locker) {
 // pause thread executing RunThread
 offline = true;
 // also kill timer if running
 if (timer.Enabled) {
 timer.Stop();
 timerElapsed = true;
 Monitor.Pulse(locker);
 }
 }
 }
 public void Online() {
 lock (locker) {
 //Pulse thread executing RunThread
 offline = false;
 Monitor.Pulse(locker);
 }
 }
 public void OfflineTooLong() {
 lock(locker) {
 this.schedule = null;
 }
 }
 public void ScheduleReady(int[] schedule) {
 lock (locker) {
 this.schedule = schedule;
 this.scheduleIndex = 0;
 Monitor.Pulse(locker);
 }
 }
 private void TimerElapsedHandler(object sender, System.Timers.ElapsedEventArgs) {
 lock(locker) {
 scheduleIndex++;
 timerElapsed = true;
 Monitor.Pulse(locker);
 }
 }
}

As you can see from the code, the intent is to "pause" the thread implementing the schedule when Offline and restart the thread when Online. Also, if you've run through all schedules(ie either schedule == null because you've been Offline too long or scheduleIndex > schedule.length), it should wait to receive a new schedule.

Any thoughts and help will be much appreciated.

• svick: Just to verify: your current code works the way you want and you're looking for improvements of the code and not for improvements of the functionality, right?
• Sandman: Hi @svick, I had posted this question with the intent of soliciting improvements in code, however now that you mention it, I wouldn't be averse to improvements in functionality either! The code I posted above was to give an idea of my functionality requirements.

Answer 1

Your code is super-comlicated for such simple logic. I'm also pretty sure it deadlocks in pretty much any use case.

class Scheduler : IDisposable
{
 private readonly object scheduleLock = new object();
 private int[] schedule = null;
 private int scheduleIndex = 0;
 private readonly ManualResetEvent collectionResetEvent = new ManualResetEvent(false);
 private readonly AutoResetEvent delayEvent = new AutoResetEvent(false);
 private volatile bool working;
 private Thread th;
 public Dispose()
 {
 Offline();
 }
 public void RunSchedule() 
 {
 while (true)
 {
 collectionResetEvent.WaitOne();
 if (!working) return;
 lock (scheduleLock)
 {
 if (schedule == null || scheduleIndex >= shedule.Count) 
 {
 collectionResetEvent.Reset();
 if (!working) return;
 }
 else
 {
 var nextSchedule = schedule[scheduleIndex++];
 //execute, 
 //probably can be done outside the lock
 }
 }
 delayEvent.WaitOne(120 * 1000);
 }
 }
 public void Offline() 
 {
 if (!working) return;
 working = false;
 delayEvent.Set();
 collectionResetEvent.Set();
 th.Join();
 th = null;
 }
 public void Online() 
 {
 if (working) return;
 working = true;
 th = new Thread(RunSchedule);
 th.Start();
 }
 public void OfflineTooLong() 
 {
 ScheduleReady(null);
 }
 public void ScheduleReady(int[] schedule) 
 {
 lock (scheduleLock) 
 {
 schedule = schedule;
 scheduleIndex = 0;
 collectionResetEvent.Set();
 delayEvent.Set();
 }
 }
}

This is just an example of how it can be refactored, to give you an idea. It can still potentially deadlock (for example if you call offline and dispose at the same time from different threads) so it requires some tweaking depending on your requirements. RunSchedule() method is somewhat hard to read, it should be splitted into sub-methods. BartoszKP makes a perfectly valid points as well, check his answer, if you haven't already.

• Sandman: @Nik- Wow! This is definitely much cleaner! Damn, I can write code, but I over-complicate things. But this is very good. Thanks a lot. Just one question though, as it happens, when Offline is called, I would like to pause the thread implementing the schedule which would mean for code like yours where you use Thread.Sleep(), I will have to call Thread.Interrupt on the thread in case the thread is sleeping. Is that not so? And so far as I understand, Thread.Interrupt should usually be avoided, right?
• Nikita B: @Sandman, yes, you should avoid using Thread methods, other then Start and Join. And you should never call Interrupt, Abort and such. As for sleeping - it will work fine when you call Offline, since after exiting sleep the thread will block on WaitHandle.WaitAll call. The issue is that to call Online again - you 'll have to actually wait for two minutes. I've modified my example, to show, how it can be avoided.
• Sandman: @BartoszKP, Nik - Apologies for going off topic, but I have to ask you something! How..I mean how, do you start getting to this point where code starts looking from what I've done to what you guys can do? What is it? Experience, knowledge, books? Suggestions, tips will be deeply appreciated! I mean, for now Nik has given me a direction in which to refactor my code, but how do I get to that point when I can start designing code like this.
• Sandman: @Nik, BartoszKP - I would understand if you guys chose not to answer this question, because it's like one of the countless "Oh..how do I become a better programmer" sort of questions! But if you choose to do, I'd be indebted!
• Sandman: In any case, I appreciate both your inputs!