kibertoad / toad-scheduler

In-memory Node.js and browser job scheduler
MIT License
564 stars 25 forks source link

Unified addJob method #229

Open jakmaz opened 6 months ago

jakmaz commented 6 months ago

Description:

This pull request simplifies the ToadScheduler by introducing a unified addJob method for job addition, deprecating the specific methods like addIntervalJob and addCronJob.

Rationale:

I chose this simple, single-method design over a polymorphic approach to maintain ease of use and understandability. The alternative considered was a more polymorphic design, where each job type would know how to add itself to the scheduler:

abstract class Job {
  abstract addToScheduler(scheduler: ToadScheduler): void;
}

class SimpleIntervalJob extends Job {
  addToScheduler(scheduler: ToadScheduler): void {
    scheduler.AddIntervalJob(this);
  }
}

Instead, I used a straightforward approach where the addJob method internally determines the job type and delegates to the appropriate method:

// Implemented approach
addJob(job: Job): void {
  if (job instanceof SimpleIntervalJob || job instanceof LongIntervalJob) {
    this.addIntervalJob(job);
  } else if (job instanceof CronJob) {
    this.addCronJob(job);
  } else {
    throw new Error('Unsupported job type.');
  }
}

This approach would follow the Open/Closed Principle more closely but was not adopted due to the stability and limited extension expected in our job types.

Impact:

The update simplifies adding jobs to the scheduler, making the codebase easier to maintain and understand while still allowing for internal specialization for different job types.

jakmaz commented 6 months ago

I think this problem implementation is beyond my skills just yet, thanks for reviewing it

kibertoad commented 6 months ago

@jakmaz I can guide you through it, if you want! You have 90% of the solution there, it just needs some small tweaks

jakmaz commented 6 months ago

Okay! I suppose I get the point of adding the id field more now. I also thought of another solution, that is adding the abstract method addToScheduler(scheduler ToadScheduler) to the Job class and implementing it in each Job type to call the correct scheduler method. Then there would be no need to use any identifiers, but it could be a bit more messy. What do you think?