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

Conversation

elicampos
Copy link

Issue: #51
I implemented a system in which it caches the searches and the individuals courses. So if you search something that was previously searched, it will still pull from the cache(Basic React Query which you guys already had), however during this process it also caches each course found. So now for each course search, it sees if the result is in the search or course cache and if neither it calls the API again. The only drawback to this is the fact that very large searches like "EEL" for example, take about 30% longer, as it's caching all the courses but again now every course with EEL pops up almost instantly.

Copy link

vercel bot commented Feb 9, 2024

@elicampos is attempting to deploy a commit to the UF OSC Team on Vercel.

A member of the Team first needs to authorize it.

Copy link

vercel bot commented Feb 10, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
swamp-scheduler ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 18, 2024 6:24pm
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
swamp-scheduler-v2 ⬜️ Ignored (Inspect) Visit Preview Mar 18, 2024 6:24pm

@UF-OSC-TechLeads
Copy link

image Run Prettier and commit :)

@UF-OSC-TechLeads UF-OSC-TechLeads linked an issue Feb 10, 2024 that may be closed by this pull request
Copy link

@UF-OSC-TechLeads UF-OSC-TechLeads left a comment

Choose a reason for hiding this comment

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

Prettier.

Copy link

@UF-OSC-TechLeads UF-OSC-TechLeads left a comment

Choose a reason for hiding this comment

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

Prettier.

Comment on lines +135 to +138
const cacheKey =
searchBy === SearchBy.COURSE_CODE
? upperPhrase
: `${searchBy}:${upperPhrase}`;
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.

app/src/scripts/soc/soc.tsx Show resolved Hide resolved
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.

Comment on lines +310 to 311
this.courseCache.set(courseCode, [course]);
}
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.

@@ -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?

Comment on lines +248 to +257
// 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();
}
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).

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.

Cache fetched courses
3 participants