Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[#11585] Fix unnecessary data read in expected submission count #11659
[#11585] Fix unnecessary data read in expected submission count #11659
Changes from 10 commits
97a5284
2646bcb
fd9ac58
08383c7
15533f6
474fecd
ffd86ba
3129653
dfc06d5
fb7e683
4041ef8
f3c2edf
0c165b2
35a490a
37dcec8
36d95bf
6692fe9
c4888b8
9e1ff59
cbcd27f
5473d99
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized, you should be able to make use of
hasFeedbackQuestionsForStudents
hereThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right... I just made those methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just removed the
giverTypeCount
method and replace with overloadedhasQnForX
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think even the overloaded method is needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other
hasFeedbackQuestionsForInstructors
hasisCreatorOfSession
which in turn fetches theFeedbackSession
entity per call. I don't feel like this is justified when we literally have thefsa
available in the caller.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then, that other method is what you should modify
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even then:
hasFeedbackQuestionsForInstructors
for each instructor is in itself already wasted. Only one kind of instructor can possibly produce different outcome and that is the creator instructor.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right about this. I should just eliminate down to only necessary information needed from db.
My take on this is that we want a snapshot of a session from one point in time. Fetching the session entity multiple times along the way might provide some inconsistency instead IMO.
Any ways we can guard against session entity not fetched from DB?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I forgot to reply this.
Probably I shouldn't mention the first reason. It is definitely a concern, but another factor being in play is what the method is supposed to do. For instance, do you want a method that "does something with this given Attribute object", or "does something with this course ID and session name"? They can very well do different things even though in a glance they sound the same.
But determining which one is the right one to use is not a black-and-white matter. It is highly situational and depends on the business logic at the point of the method call. Which is why, "[session] entity not fetched from DB" is not even necessarily a problem.
But the second reason still stands as to why there is still room for even more optimization here.