Refactor sync loop and update job schedule to hourly#102
Refactor sync loop and update job schedule to hourly#102afshinalizadeh merged 3 commits intomainfrom
Conversation
Refactored CrystaProgramSyncJobRunner to use a while loop with index-based access instead of foreach for module iteration. Updated the Hangfire recurring job schedule for "crysta-program-sync" to run at the start of every hour instead of every minute.
…yncall Refactor sync loop and update job schedule to hourly
There was a problem hiding this comment.
Pull request overview
This PR refactors the CrystaProgramSyncJobRunner's module iteration logic and updates the Hangfire recurring job schedule. While the PR description states the job will run "hourly", the cron expression change requires clarification, and the loop refactoring reduces code maintainability without providing any benefit.
Key changes:
- Changed cron expression from
*/1 * * * *to0 */1 * * *in both Server.Api and Server.Web - Refactored foreach loop to while loop with index-based access in CrystaProgramSyncJobRunner
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/Server/CrystaLearn.Server.Web/Program.Middlewares.cs |
Updated Hangfire cron expression for the crysta-program-sync job |
src/Server/CrystaLearn.Server.Api/Program.Middlewares.cs |
Updated Hangfire cron expression for the crysta-program-sync job |
src/Core/CrystaLearn.Core/Services/Jobs/CrystaProgramSyncJobRunner.cs |
Refactored module iteration from foreach to while loop with index counter |
| int i = 0; | ||
| while (i < modules.Count) | ||
| { | ||
| await syncService.SyncAsync(module); | ||
| await syncService.SyncAsync(modules[i]); | ||
| i++; |
There was a problem hiding this comment.
The refactoring from a foreach loop to a while loop with index-based access is unnecessary and reduces code readability. The original foreach loop was more idiomatic and clearer in intent. Since modules is a List, both approaches have the same performance characteristics, making this change purely detrimental to maintainability.
| recurringJobId: "crysta-program-sync", | ||
| methodCall: x => x.RunSyncForAllModules(CancellationToken.None), | ||
| cronExpression: "*/1 * * * *"); | ||
| cronExpression: "0 */1 * * *"); |
There was a problem hiding this comment.
The cron expression "0 */1 * * *" does not achieve hourly execution as described in the PR. This expression means "at minute 0 of every hour", which is equivalent to "0 * * * " and would run at the start of each hour (e.g., 1:00, 2:00, 3:00). However, "/1" in the hour field is redundant. If the intention is truly hourly execution at the start of each hour, use "0 * * * " instead. If the original "/1 * * * *" (every minute) was intended to be changed to every hour, the correct expression is "0 * * * *".
| cronExpression: "0 */1 * * *"); | |
| cronExpression: "0 * * * *"); |
| recurringJobId: "crysta-program-sync", | ||
| methodCall: x => x.RunSyncForAllModules(CancellationToken.None), | ||
| cronExpression: "*/1 * * * *"); | ||
| cronExpression: "0 */1 * * *"); |
There was a problem hiding this comment.
The cron expression "0 */1 * * *" does not achieve hourly execution as described in the PR. This expression means "at minute 0 of every hour", which is equivalent to "0 * * * " and would run at the start of each hour (e.g., 1:00, 2:00, 3:00). However, "/1" in the hour field is redundant. If the intention is truly hourly execution at the start of each hour, use "0 * * * " instead. If the original "/1 * * * *" (every minute) was intended to be changed to every hour, the correct expression is "0 * * * *".
| cronExpression: "0 */1 * * *"); | |
| cronExpression: "0 * * * *"); |
Refactored CrystaProgramSyncJobRunner to use a while loop with index-based access instead of foreach for module iteration. Updated the Hangfire recurring job schedule for "crysta-program-sync" to run at the start of every hour instead of every minute.