Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unified addJob method #229

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Unified addJob method #229

wants to merge 1 commit into from

Conversation

jakmaz
Copy link

@jakmaz jakmaz commented Apr 16, 2024

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.

Allows to add the new job of any type to the scheduler
@@ -54,6 +66,16 @@ export class ToadScheduler {
this.engines.cronJobEngine.add(job)
}

addJob(job: Job): void {
if (job instanceof SimpleIntervalJob || job instanceof LongIntervalJob) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instanceof check is notoriously unrealiable and will not work correctly when comparing instances created in a different realm (e. g. if you are using a job created inside a shared library in an app that depends both on a shared library and on toad-scheduler). I would recommend introducing some id field on the job, and using it for a proper type guard

Copy link
Owner

@kibertoad kibertoad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test is missing

@jakmaz
Copy link
Author

jakmaz commented Apr 27, 2024

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

@jakmaz jakmaz closed this Apr 27, 2024
@kibertoad
Copy link
Owner

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

@kibertoad kibertoad reopened this Apr 27, 2024
@jakmaz
Copy link
Author

jakmaz commented Apr 28, 2024

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants