HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Custom Task Scheduler

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
customtaskscheduler

Problem

I want a task scheduler with some specific capabilities for my application. I've searched for a good library that provides me with the following, but found nothing.

  • Queuing awaitable tasks.



  • Ability to stop started jobs.



  • Ability to repeat some jobs.



  • Ability to cancel repeat enabled jobs.



  • Specifying a priority foreach schedule.



  • Running schedulers on a separate thread [optionable].



  • Add Task and Task



So, I've decided to create my own and I want you to review my code for improvements:

  • Detailed review of my code for errors, drawbacks.



  • How I can make my code run every scheduler on a separate thread in an efficient way.



  • Any more suggestions to help making my code more reliable and more professional, along with new features.



```
public static class TaskManager
{
private static List _schedulers;

///
/// Gets a list of currently running schedulers.
///
public static IEnumerable RunningSchedulers
{
get { return _schedulers.Where(scheduler => scheduler.IsRunning()); }
}

///
/// Gets all scheduleres.
///
public static IEnumerable AllSchedulers(string name)
{
return _schedulers;
}

///
/// Gets a scheduler by it's name.
///
public static Scheduler GetScheduler(string name)
{
return _schedulers.Find(scheduler => scheduler.Name == name);
}

///
/// Attempts to remove a scheduler from the list.
///
public static bool RemoveScheduler(Scheduler scheduler)
{
return _schedulers.Remove(scheduler);
}

///
/// Creates a new scheduler and adds it the the list.
///
public static Scheduler CreateScheduler(int priority, string name = null)
{
if (_schedulers == null)
_schedulers = new List();

var scheduler = new Scheduler(priority, name);
_schedulers.Add(scheduler);
return scheduler;
}
}

public class Scheduler : Queue
{
public string Name { get;

Solution

The issues i see off the bat:

1) In my experience this Scheduler : Queue is almost always a bad idea. If you are not directly overriding a Queue class methods - there is no reason to derive Scheduler from it. Futhermore, adding/removing schedules will cause exceptions, if it happens at the same time, as you enumerate your queue in timer callback. You should use aggregation instead and synchronize your queue.

2) You should extract IJob interface. And use it in Scheduler. Because, for example, some guy might want to use your scheduler but he might not want to use Task class at all. Why force him? You could declare IJob like this (for example):

interface IJob : IDisposable
{
    string Name {get;}
    DateTime ExecutionTime {get;}

    void Start();
    void Cancel();

    bool Repeat {get;}
    bool InProgress {get;}

    event Action Started;
    event Action Completed;
    event Action ProgressChanged;
}


It will give your scheduler and task manager all the control they need, without mentioning tasks at all. While you can still use tasks in your base implementation (Job class).

3) Same goes for IScheduler.

4) Methods such as public Job TriggerEvery(int interval), public Job Seconds(), etc. do not belong to Job class and will only lead to confusion. There are better ways. You can declare them as extension methods. You can implement job factory and impelent those expressions there. Either way you should avoid forcing a guy, who is going to use your code, to use those expressions. You should allow him to simply set _interval (via property or constructor).

5) private int _interval; what is int? Is it seconds? Is it ticks? Is it milliseconds? No way to tell without digging into your code. Use better naming, or use TimeSpan.

6) Job.StartRepeating(), Job.StopRepeating() - those do not belong to job class either. Job class should have an indicator whether the task should be repeated forever or not and thats it. It should not run itself or repeat itself - that is the job for scheduler/task manager. Not to mention that im not even sure, what those methods do and if they work.

7) You should never call events like that Trigger(this, job);. You can check Jeffrey Richter's books if you need details.

8) public bool IsRunning() this should be a property.

9) The idea behind your TaskManager is not quite clear. At the moment it looks like an overcomplicated dictionary.

10) Your Jobs are not being executed at any point. This is counter intuitive. If i add my job to scheduler/task manager, i expect it to run at designed time. Instead of triggering an event and let some external code handle the task itself, you should run the task with your scheduler/task manager instead (using the interface i suggested in (2) or your own). Because thats the whole point, no? You can still generate an event tho.

Code Snippets

interface IJob : IDisposable
{
    string Name {get;}
    DateTime ExecutionTime {get;}

    void Start();
    void Cancel();

    bool Repeat {get;}
    bool InProgress {get;}

    event Action<IJob> Started;
    event Action<IJob> Completed;
    event Action<IJob, int> ProgressChanged;
}

Context

StackExchange Code Review Q#28760, answer score: 4

Revisions (0)

No revisions yet.