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

Add Couse Cache System #69

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 47 additions & 6 deletions app/src/scripts/soc/soc.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,16 @@ interface UID {
export abstract class SOC_Generic {
info: SOCInfo;
courses: Course[];
// Initialize cache for broad search results and individual courses
courseCache: Map<string, Course[]>;

/* CONSTRUCT & INITIALIZE */

protected constructor(info: SOCInfo, courses: Course[]) {
this.info = info;
this.courses = courses;
// Initialize the cache as an empty Map
this.courseCache = new Map();
}

static async initialize(): Promise<SOC_Generic> {
Expand Down Expand Up @@ -125,22 +129,46 @@ export abstract class SOC_Generic {
return [];

const upperPhrase: string = phrase.toUpperCase();

// Check cache first for both broad and specific course searches
// Use course code as cache key for specific searches
const cacheKey =
searchBy === SearchBy.COURSE_CODE
? upperPhrase
: `${searchBy}:${upperPhrase}`;
Comment on lines +135 to +138
Copy link
Collaborator

Choose a reason for hiding this comment

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

The cacheKey can always be ${searchBy}:${upperPhrase} as it is a unique identifier.

if (this.courseCache.has(cacheKey)) {
console.log("Returning cached results for:", cacheKey);
// Return cached results
return this.courseCache.get(cacheKey) || [];
RobertConde marked this conversation as resolved.
Show resolved Hide resolved
}

let results: Course[] = [];
if (searchBy === SearchBy.COURSE_CODE) {
return this.courses.filter((c) => c.code.includes(upperPhrase));
results = this.courses.filter((c) => c.code.includes(upperPhrase));
// Cache each individual course from the broad search result
results.forEach((course) =>
this.courseCache.set(course.code.toUpperCase(), [course]),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can lead to some nasty bugs with distinct courses that share the same course code (e.g. quest courses & special topics).

You cannot assume that a particular course code belongs to only one course.

You should only cache after searching for all possible courses as there may be more than one.

);
} else if (searchBy === SearchBy.COURSE_TITLE) {
return this.courses.filter((c) =>
results = this.courses.filter((c) =>
c.name.toUpperCase().includes(upperPhrase),
);
} else if (searchBy === SearchBy.INSTRUCTOR) {
return this.courses.filter((c) =>
results = this.courses.filter((c) =>
c.sections.some((s) =>
s.instructors.some((inst) =>
inst.toUpperCase().includes(upperPhrase),
),
),
);
} else {
throw new Error("Unhandled SearchBy.");
}
throw new Error("Unhandled SearchBy.");

// Store broad search results in cache
this.courseCache.set(cacheKey, results);

return results;
}

/* UTILS */
Expand Down Expand Up @@ -217,6 +245,16 @@ export class SOC_API extends SOC_Generic {
lcn = 0,
): Promise<void> {
if (!phrase) return Promise.resolve();
// Check cache for individual course code to avoid refetching
const upperPhrase = phrase.toUpperCase();
if (
searchBy === SearchBy.COURSE_CODE &&
this.courseCache.has(upperPhrase)
) {
console.log("Course already fetched and cached:", upperPhrase);
// Exit if the specific course is already cached
return Promise.resolve();
}
Comment on lines +248 to +257
Copy link
Collaborator

Choose a reason for hiding this comment

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

fetchCourse(...) should only be called when needed, hence you don't need to check if the thing you're looking for is cached (i.e. it is the responsibility of the calling function to determine whether or not to call fetchCourses to fetch).


const search = JSON.stringify({ by: searchBy, phrase });
if (this.fetchCache.has(search)) {
Expand All @@ -243,7 +281,7 @@ export class SOC_API extends SOC_Generic {
// Add each course, if appropriate
COURSES.forEach((courseJson: API_Course) => {
const courseID: string = courseJson.courseId,
courseCode: string = courseJson.code,
courseCode: string = courseJson.code.toUpperCase(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Q: Did you find course codes that were not in uppercase?

courseInd: number = this.courses.length;

// Prevent duplicates
Expand All @@ -266,7 +304,10 @@ export class SOC_API extends SOC_Generic {
);
},
);
this.courses.push(course); // Add the course to the courses array
// Add the course to the courses array
this.courses.push(course);
// Cache the course by its code
this.courseCache.set(courseCode, [course]);
}
Comment on lines +310 to 311
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't assume this.

});

Expand Down