SAK-52310 SelectActionListener refactor for performance while accessing the assessment list as a student#14350
Open
jumarub wants to merge 2 commits intosakaiproject:masterfrom
Open
SAK-52310 SelectActionListener refactor for performance while accessing the assessment list as a student#14350jumarub wants to merge 2 commits intosakaiproject:masterfrom
jumarub wants to merge 2 commits intosakaiproject:masterfrom
Conversation
…ng the assessment list as a student https://sakaiproject.atlassian.net/browse/SAK-52310
There was a problem hiding this comment.
Pull request overview
This pull request refactors the SelectActionListener class for performance improvements when students access the assessment list. The refactoring focuses on code modernization, improved type safety, and cleaner logic.
Changes:
- Added generic type parameters to collections (Map, List) for improved type safety
- Replaced traditional for-loops with enhanced for-loops and Stream API operations
- Simplified conditional logic by consolidating if-else blocks into single-line assignments
- Improved code formatting and removed trailing whitespace
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...o-app/src/java/org/sakaiproject/tool/assessment/ui/listener/select/SelectActionListener.java
Show resolved
Hide resolved
...o-app/src/java/org/sakaiproject/tool/assessment/ui/listener/select/SelectActionListener.java
Show resolved
Hide resolved
...o-app/src/java/org/sakaiproject/tool/assessment/ui/listener/select/SelectActionListener.java
Outdated
Show resolved
Hide resolved
Contributor
WalkthroughRefactors SelectActionListener.java to use typed generics, enhanced for-loops and streams; consolidates past-due/updated/multiple-submission logic; adds average-score aggregation and reviewable/recorded grouping; updates secure-delivery URL resolution and introduces a new public setter on SelectAssessmentBean for average submission tracking. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/listener/select/SelectActionListener.java`:
- Around line 293-310: DeliveryBeanie.getFinalScore() can be null causing
Double.parseDouble(null) to NPE; before parsing inside the loop over
averageScoreAssessmentGradingList check if db.getFinalScore() is null or empty
and skip that entry (continue) or treat it as 0 per desired semantics, then
parse the non-null value (e.g., Double.parseDouble(db.getFinalScore())); also
when computing averageScore for lastPublishedAssessmentId ensure
totalSubmissions > 0 before dividing and only put an average into
averageScoreMap when totalSubmissions is positive; update references in this
block (DeliveryBeanie, getFinalScore, averageScoreAssessmentGradingList,
averageScoreMap, lastPublishedAssessmentId, totalScores, totalSubmissions)
accordingly.
- Around line 359-365: The lookup averageScoreMap.get(assessmentIdNew) can be
null; update the block in SelectActionListener where processRecordedAvg is true
(around variables assessmentIdNew, beanie.getAssessmentId(), averageScoreMap and
recorded.setFinalScore/setGrade/setRawScore) to first retrieve the value into a
local (e.g., avg) and test for null before calling toString(); if avg is
non-null convert to string and call recorded.setFinalScore/setGrade/setRawScore,
otherwise avoid calling toString() and either leave the recorded fields unset or
explicitly set them to null/empty as appropriate for downstream logic.
🧹 Nitpick comments (1)
samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/listener/select/SelectActionListener.java (1)
332-357: Consider extracting a copy/clone utility for DeliveryBeanie.The manual field-by-field copying is verbose and error-prone if new fields are added to
DeliveryBeanie. A copy constructor or static factory method would centralize this logic.
Comment on lines
+293
to
310
| for (DeliveryBeanie db : averageScoreAssessmentGradingList) { | ||
| String currentId = db.getAssessmentId(); | ||
| double score = Double.parseDouble(db.getFinalScore()); | ||
|
|
||
| if (currentId.equals(lastPublishedAssessmentId)) { | ||
| totalScores += score; | ||
| totalSubmissions++; | ||
| } else { | ||
| if (lastPublishedAssessmentId != null) { | ||
| double averageScore = totalScores / totalSubmissions; | ||
| averageScoreMap.put(lastPublishedAssessmentId, averageScore); | ||
| } | ||
|
|
||
| lastPublishedAssessmentId = currentId; | ||
| totalScores = score; | ||
| totalSubmissions = 1; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Potential NPE when parsing finalScore.
If AssessmentGradingData.getFinalScore() was null (see lines 242-249 where setFinalScore is conditional), then db.getFinalScore() returns null and Double.parseDouble(null) throws NullPointerException.
🐛 Proposed fix with null guard
for (DeliveryBeanie db : averageScoreAssessmentGradingList) {
String currentId = db.getAssessmentId();
- double score = Double.parseDouble(db.getFinalScore());
+ String finalScoreStr = db.getFinalScore();
+ if (finalScoreStr == null) {
+ continue;
+ }
+ double score = Double.parseDouble(finalScoreStr);
if (currentId.equals(lastPublishedAssessmentId)) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (DeliveryBeanie db : averageScoreAssessmentGradingList) { | |
| String currentId = db.getAssessmentId(); | |
| double score = Double.parseDouble(db.getFinalScore()); | |
| if (currentId.equals(lastPublishedAssessmentId)) { | |
| totalScores += score; | |
| totalSubmissions++; | |
| } else { | |
| if (lastPublishedAssessmentId != null) { | |
| double averageScore = totalScores / totalSubmissions; | |
| averageScoreMap.put(lastPublishedAssessmentId, averageScore); | |
| } | |
| lastPublishedAssessmentId = currentId; | |
| totalScores = score; | |
| totalSubmissions = 1; | |
| } | |
| } | |
| for (DeliveryBeanie db : averageScoreAssessmentGradingList) { | |
| String currentId = db.getAssessmentId(); | |
| String finalScoreStr = db.getFinalScore(); | |
| if (finalScoreStr == null) { | |
| continue; | |
| } | |
| double score = Double.parseDouble(finalScoreStr); | |
| if (currentId.equals(lastPublishedAssessmentId)) { | |
| totalScores += score; | |
| totalSubmissions++; | |
| } else { | |
| if (lastPublishedAssessmentId != null) { | |
| double averageScore = totalScores / totalSubmissions; | |
| averageScoreMap.put(lastPublishedAssessmentId, averageScore); | |
| } | |
| lastPublishedAssessmentId = currentId; | |
| totalScores = score; | |
| totalSubmissions = 1; | |
| } | |
| } |
🤖 Prompt for AI Agents
In
`@samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/listener/select/SelectActionListener.java`
around lines 293 - 310, DeliveryBeanie.getFinalScore() can be null causing
Double.parseDouble(null) to NPE; before parsing inside the loop over
averageScoreAssessmentGradingList check if db.getFinalScore() is null or empty
and skip that entry (continue) or treat it as 0 per desired semantics, then
parse the non-null value (e.g., Double.parseDouble(db.getFinalScore())); also
when computing averageScore for lastPublishedAssessmentId ensure
totalSubmissions > 0 before dividing and only put an average into
averageScoreMap when totalSubmissions is positive; update references in this
block (DeliveryBeanie, getFinalScore, averageScoreAssessmentGradingList,
averageScoreMap, lastPublishedAssessmentId, totalScores, totalSubmissions)
accordingly.
Comment on lines
+359
to
+365
| if (processRecordedAvg) { | ||
| assessmentIdNew = beanie.getAssessmentId(); | ||
| String avgScoreStr = averageScoreMap.get(assessmentIdNew).toString(); | ||
|
|
||
| recorded.setFinalScore(avgScoreStr); | ||
| recorded.setGrade(avgScoreStr); | ||
| recorded.setRawScore(avgScoreStr); |
Contributor
There was a problem hiding this comment.
Add defensive null check for map lookup.
If an assessment's finalScore was null and skipped during average calculation, averageScoreMap.get(assessmentIdNew) would return null, causing NPE on .toString().
🛡️ Proposed defensive fix
if (processRecordedAvg) {
assessmentIdNew = beanie.getAssessmentId();
- String avgScoreStr = averageScoreMap.get(assessmentIdNew).toString();
+ Double avgScore = averageScoreMap.get(assessmentIdNew);
+ if (avgScore == null) {
+ continue;
+ }
+ String avgScoreStr = avgScore.toString();
recorded.setFinalScore(avgScoreStr);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (processRecordedAvg) { | |
| assessmentIdNew = beanie.getAssessmentId(); | |
| String avgScoreStr = averageScoreMap.get(assessmentIdNew).toString(); | |
| recorded.setFinalScore(avgScoreStr); | |
| recorded.setGrade(avgScoreStr); | |
| recorded.setRawScore(avgScoreStr); | |
| if (processRecordedAvg) { | |
| assessmentIdNew = beanie.getAssessmentId(); | |
| Double avgScore = averageScoreMap.get(assessmentIdNew); | |
| if (avgScore == null) { | |
| continue; | |
| } | |
| String avgScoreStr = avgScore.toString(); | |
| recorded.setFinalScore(avgScoreStr); | |
| recorded.setGrade(avgScoreStr); | |
| recorded.setRawScore(avgScoreStr); |
🤖 Prompt for AI Agents
In
`@samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/listener/select/SelectActionListener.java`
around lines 359 - 365, The lookup averageScoreMap.get(assessmentIdNew) can be
null; update the block in SelectActionListener where processRecordedAvg is true
(around variables assessmentIdNew, beanie.getAssessmentId(), averageScoreMap and
recorded.setFinalScore/setGrade/setRawScore) to first retrieve the value into a
local (e.g., avg) and test for null before calling toString(); if avg is
non-null convert to string and call recorded.setFinalScore/setGrade/setRawScore,
otherwise avoid calling toString() and either leave the recorded fields unset or
explicitly set them to null/empty as appropriate for downstream logic.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/listener/select/SelectActionListener.java`:
- Around line 284-315: The current aggregation in SelectActionListener uses a
sequential scan over averageScoreAssessmentGradingList and relies on consecutive
grouping by assessment id (lastPublishedAssessmentId), which breaks when the
list is not pre-sorted; change the logic to be order-independent by grouping
submissions by assessment id and computing averages per group: use a Map keyed
by DeliveryBeanie.getAssessmentId() (e.g., via
stream().collect(Collectors.groupingBy(DeliveryBeanie::getAssessmentId))) or
iterate and accumulate totals/counts into a Map<String, double[]> or holder
object, then compute averageScoreMap entries from those aggregates and populate
averageScoreMap accordingly (keep variable names
averageScoreAssessmentGradingList, averageScoreMap, DeliveryBeanie,
getAssessmentId, getFinalScore).
- Around line 236-259: The call to hasFeedback(p, g.getFinalScore()) can NPE
because g.getFinalScore() may be null; before invoking hasFeedback (just after
PublishedAssessmentFacade p = ... and prior to
delivery.setFeedback(hasFeedback)), add a null guard that checks
g.getFinalScore() and only calls hasFeedback when non-null (or call an
overload/alternate logic when null), then set delivery.setFeedback to the safe
result (e.g., a default "no feedback" value or result of hasFeedback when score
exists); ensure you reference g.getFinalScore(), hasFeedback(...),
PublishedAssessmentFacade p, and delivery.setFeedback(...) when making the
change.
Comment on lines
+236
to
+259
| delivery.setFeedbackDelivery(getFeedbackDelivery(g.getPublishedAssessmentId(), publishedAssessmentHash)); | ||
| delivery.setFeedbackComponentOption(getFeedbackComponentOption(g.getPublishedAssessmentId(), publishedAssessmentHash)); | ||
| delivery.setFeedbackDate(getFeedbackDate(g.getPublishedAssessmentId(), publishedAssessmentHash)); | ||
| delivery.setFeedbackEndDate(getFeedbackEndDate(g.getPublishedAssessmentId(), publishedAssessmentHash)); | ||
| delivery.setFeedbackScoreThreshold(getFeedbackScoreThreshold(g.getPublishedAssessmentId(), publishedAssessmentHash)); | ||
|
|
||
| delivery.setIsRecordedAssessment(g.getIsRecorded()); | ||
|
|
||
| // to do: set statistics and time for delivery here. | ||
| submittedAssessmentGradingList.add(delivery); | ||
| if (g.getFinalScore() != null) { | ||
| String scoreStr = g.getFinalScore().toString(); | ||
|
|
||
| delivery.setFinalScore(scoreStr); | ||
| delivery.setGrade(scoreStr); | ||
| delivery.setRawScore(scoreStr); // Bug 318 fix. It seems raw score should also be based on final score. | ||
| delivery.setRaw(g.getFinalScore().longValue()); | ||
| } | ||
|
|
||
| delivery.setTimeElapse(getTimeElapsed(g.getTimeElapsed())); | ||
| delivery.setSubmissionDate(g.getSubmittedDate()); | ||
| delivery.setHasAssessmentBeenModified(getHasAssessmentBeenModified(select, g, publishedAssessmentHash)); | ||
| delivery.setSubmitted(true); // records are all submitted for grade | ||
|
|
||
| PublishedAssessmentFacade p = (PublishedAssessmentFacade)publishedAssessmentHash.get(g.getPublishedAssessmentId()); | ||
| // check is feedback is available | ||
| String hasFeedback = hasFeedback(p, g.getFinalScore()); | ||
| delivery.setFeedback(hasFeedback); |
Contributor
There was a problem hiding this comment.
Guard hasFeedback against null finalScore.
g.getFinalScore() can be null, and the feedback‑threshold path dereferences it, causing NPEs. Add a null guard before calling hasFeedback.
🐛 Proposed fix
- delivery.setFeedbackScoreThreshold(getFeedbackScoreThreshold(g.getPublishedAssessmentId(), publishedAssessmentHash));
+ Double feedbackScoreThreshold = getFeedbackScoreThreshold(g.getPublishedAssessmentId(), publishedAssessmentHash);
+ delivery.setFeedbackScoreThreshold(feedbackScoreThreshold);
@@
- String hasFeedback = hasFeedback(p, g.getFinalScore());
+ Double finalScore = g.getFinalScore();
+ String hasFeedback = (finalScore == null && feedbackScoreThreshold != null)
+ ? "blank"
+ : hasFeedback(p, finalScore);
delivery.setFeedback(hasFeedback);🤖 Prompt for AI Agents
In
`@samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/listener/select/SelectActionListener.java`
around lines 236 - 259, The call to hasFeedback(p, g.getFinalScore()) can NPE
because g.getFinalScore() may be null; before invoking hasFeedback (just after
PublishedAssessmentFacade p = ... and prior to
delivery.setFeedback(hasFeedback)), add a null guard that checks
g.getFinalScore() and only calls hasFeedback when non-null (or call an
overload/alternate logic when null), then set delivery.setFeedback to the safe
result (e.g., a default "no feedback" value or result of hasFeedback when score
exists); ensure you reference g.getFinalScore(), hasFeedback(...),
PublishedAssessmentFacade p, and delivery.setFeedback(...) when making the
change.
Comment on lines
+284
to
+315
| List<DeliveryBeanie> averageScoreAssessmentGradingList = submittedAssessmentGradingList.stream() | ||
| .filter(db -> EvaluationModelIfc.AVERAGE_SCORE.toString().equals(db.getScoringOption())) | ||
| .collect(Collectors.toList()); | ||
|
|
||
| String lastPublishedAssessmentId = null; | ||
| Map<String, Double> averageScoreMap = new HashMap<>(); | ||
| double totalScores = 0d; | ||
| int totalSubmissions = 0; | ||
|
|
||
| for (DeliveryBeanie db : averageScoreAssessmentGradingList) { | ||
| String currentId = db.getAssessmentId(); | ||
| double score = Double.parseDouble(db.getFinalScore()); | ||
|
|
||
| if (currentId.equals(lastPublishedAssessmentId)) { | ||
| totalScores += score; | ||
| totalSubmissions++; | ||
| } else { | ||
| if (lastPublishedAssessmentId != null) { | ||
| double averageScore = totalScores / totalSubmissions; | ||
| averageScoreMap.put(lastPublishedAssessmentId, averageScore); | ||
| } | ||
|
|
||
| lastPublishedAssessmentId = currentId; | ||
| totalScores = score; | ||
| totalSubmissions = 1; | ||
| } | ||
| } | ||
|
|
||
| String lastPublishedAssessmentId = ""; | ||
| Map averageScoreMap = new HashMap(); | ||
| double totalScores= 0d; | ||
| int totalSubmissions= 0; | ||
| double averageScore = 0d; | ||
|
|
||
| for (int i = 0; i < averageScoreAssessmentGradingList.size(); i++) | ||
| { | ||
| DeliveryBeanie db = (DeliveryBeanie) averageScoreAssessmentGradingList.get(i); | ||
| if ((lastPublishedAssessmentId != null && lastPublishedAssessmentId.equals(db.getAssessmentId())) || averageScoreAssessmentGradingList.size() == 1) { | ||
| totalScores += Double.parseDouble(db.getFinalScore()); | ||
| totalSubmissions++; | ||
| if (i == averageScoreAssessmentGradingList.size() - 1) { | ||
| averageScore = totalScores/totalSubmissions; | ||
| averageScoreMap.put(db.getAssessmentId(), averageScore); | ||
| } | ||
| } | ||
| else { | ||
| if (i > 0) { | ||
| averageScore = totalScores/totalSubmissions; | ||
| averageScoreMap.put(lastPublishedAssessmentId, averageScore); | ||
| } | ||
| lastPublishedAssessmentId = db.getAssessmentId(); | ||
| totalScores = Double.parseDouble(db.getFinalScore()); | ||
| totalSubmissions = 1; | ||
|
|
||
| if (i == averageScoreAssessmentGradingList.size() - 1) { | ||
| averageScore = totalScores/totalSubmissions; | ||
| averageScoreMap.put(db.getAssessmentId(), averageScore); | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
| if (lastPublishedAssessmentId != null) { | ||
| double averageScore = totalScores / totalSubmissions; | ||
| averageScoreMap.put(lastPublishedAssessmentId, averageScore); | ||
| } |
Contributor
There was a problem hiding this comment.
Average‑score aggregation currently depends on list ordering.
Sorting by title can interleave assessments that share a title, which can skew averages. Aggregate by assessmentId directly to make results order‑independent.
🔧 Proposed fix
- String lastPublishedAssessmentId = null;
- Map<String, Double> averageScoreMap = new HashMap<>();
- double totalScores = 0d;
- int totalSubmissions = 0;
-
- for (DeliveryBeanie db : averageScoreAssessmentGradingList) {
- String currentId = db.getAssessmentId();
- double score = Double.parseDouble(db.getFinalScore());
-
- if (currentId.equals(lastPublishedAssessmentId)) {
- totalScores += score;
- totalSubmissions++;
- } else {
- if (lastPublishedAssessmentId != null) {
- double averageScore = totalScores / totalSubmissions;
- averageScoreMap.put(lastPublishedAssessmentId, averageScore);
- }
-
- lastPublishedAssessmentId = currentId;
- totalScores = score;
- totalSubmissions = 1;
- }
- }
-
- if (lastPublishedAssessmentId != null) {
- double averageScore = totalScores / totalSubmissions;
- averageScoreMap.put(lastPublishedAssessmentId, averageScore);
- }
+ Map<String, Double> averageScoreMap = averageScoreAssessmentGradingList.stream()
+ .collect(Collectors.groupingBy(
+ DeliveryBeanie::getAssessmentId,
+ Collectors.averagingDouble(db -> Double.parseDouble(db.getFinalScore()))
+ ));🤖 Prompt for AI Agents
In
`@samigo/samigo-app/src/java/org/sakaiproject/tool/assessment/ui/listener/select/SelectActionListener.java`
around lines 284 - 315, The current aggregation in SelectActionListener uses a
sequential scan over averageScoreAssessmentGradingList and relies on consecutive
grouping by assessment id (lastPublishedAssessmentId), which breaks when the
list is not pre-sorted; change the logic to be order-independent by grouping
submissions by assessment id and computing averages per group: use a Map keyed
by DeliveryBeanie.getAssessmentId() (e.g., via
stream().collect(Collectors.groupingBy(DeliveryBeanie::getAssessmentId))) or
iterate and accumulate totals/counts into a Map<String, double[]> or holder
object, then compute averageScoreMap entries from those aggregates and populate
averageScoreMap accordingly (keep variable names
averageScoreAssessmentGradingList, averageScoreMap, DeliveryBeanie,
getAssessmentId, getFinalScore).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
https://sakaiproject.atlassian.net/browse/SAK-52310
Summary by CodeRabbit
Refactor
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.