Skip to content

Feat/scheduler#111

Merged
tyu012 merged 3 commits intodevelopmentfrom
feat/scheduler
May 8, 2025
Merged

Feat/scheduler#111
tyu012 merged 3 commits intodevelopmentfrom
feat/scheduler

Conversation

@khanhdo05
Copy link
Copy Markdown
Collaborator

Non-breaking changes, can be merged into dev then keep working on scheduling logic.

@khanhdo05 khanhdo05 requested a review from tyu012 May 8, 2025 05:08
Copy link
Copy Markdown
Collaborator

@tyu012 tyu012 left a comment

Choose a reason for hiding this comment

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

I am leaving some comments that might want to be addressed.

console.warn("This event does not have end time, eventId: " + event.id);
continue;
}
total_events_duration_for_a_deadline += UserPreferenceUtils.dateToMinuteNumber(event.end_time) - UserPreferenceUtils.dateToMinuteNumber(event.start_time);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We can just perform this operation with the Date objects: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date#calculating_elapsed_time

This would resolve the edge case of whether an event spans across midnight.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Also, remember to use camel case in TypeScript

const dayEnd = userPreferences.endTime;
const minIncrement = 5;

while (UserPreferenceUtils.dateToMinuteNumber(checkTime) <= dayEnd) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can use a for loop

currentMinutes >= userPreferences.startTime &&
currentMinutes + userPreferences.workDuration <= userPreferences.endTime
);
currentMinutes + userPreferences.workDuration <= userPreferences.endTime;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I am not sure if we need to add workDuration. Maybe a minimum work duration of 5 minutes instead as a workaround?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

then we are not considering the work duration at all. i guess we can do + min(workDuration, projected_duration)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Changed to the following:

const isWithinWorkHours =
      currentMinutes >= userPreferences.startTime &&
      currentMinutes <= userPreferences.endTime;

event &&
(event.start_time == undefined ||
event.end_time == undefined ||
(event.start_time <= time && event.end_time > time))
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If start time or end time is undefined, I think that we should exclude the event from the events at a given time.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I might have written this wrong when I initially wrote this.

);
}

return dayEnd - currentMinutes;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

In general about this function, this is not filtering by events that occur within the current day first.

* @param deadlineId A deadline_id to match to event
* @private
*/
private static getEventByDeadlineId(events: UserEvent[], deadlineId: string) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can specify return type now

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Also, functions can be renamed getEventsByDeadlineId with events being plural since we are returning multiple events.

const freeBlock = this.nextFreeTimeBlock([...events, ...scheduledEvents], userPreferences, currentTime);

// No available block left today
if (!freeBlock) break;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We should probably try again at the next day (if the deadline is not due by then).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We didn't ask which work day users prefer, so should we default to M-F or 7 days a week (the latter is more useful for college students in my opinion)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The latter

userPreferences: UserPreferences
): SchedulerOutput {
const scheduledEvents: UserEvent[] = [];
const deadlineRemainders = this.calculateDeadlineRemainders(events, deadlines);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Sorting by priority may be useful

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

oh true i forgot to consider that field completely

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I will leave this as a to-do since priority is a string

Copy link
Copy Markdown
Collaborator

@tyu012 tyu012 left a comment

Choose a reason for hiding this comment

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

I have implemented most of the commented changes; I will approve the PR.

userPreferences: UserPreferences
): SchedulerOutput {
const scheduledEvents: UserEvent[] = [];
const deadlineRemainders = this.calculateDeadlineRemainders(events, deadlines);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I will leave this as a to-do since priority is a string

currentMinutes >= userPreferences.startTime &&
currentMinutes + userPreferences.workDuration <= userPreferences.endTime
);
currentMinutes + userPreferences.workDuration <= userPreferences.endTime;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Changed to the following:

const isWithinWorkHours =
      currentMinutes >= userPreferences.startTime &&
      currentMinutes <= userPreferences.endTime;

@tyu012
Copy link
Copy Markdown
Collaborator

tyu012 commented May 8, 2025

I have implemented most of the commented changes; I will approve the PR.

@tyu012 tyu012 merged commit a078e40 into development May 8, 2025
1 check passed
@trappaly trappaly deleted the feat/scheduler branch May 14, 2025 23:03
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