⚡ Bolt: O(n) calculation optimization in Dashboard - #150
Conversation
Combined `.map()`, `Math.max()`, and `.forEach()` into a single loop in `updateStatistics` to reduce array allocations and iteration overhead. Co-authored-by: alvin000009238 <107313913+alvin000009238@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
There was a problem hiding this comment.
Code Review
This pull request optimizes the updateStatistics function by merging the maximum score calculation and weighted average computation into a single loop, reducing array allocations and preventing potential stack overflow issues. Feedback suggests using a for...of loop for better readability and employing Math.max within the loop to ensure idiomatic code and consistent NaN handling.
| for (let i = 0; i < subjects.length; i++) { | ||
| const subject = subjects[i]; | ||
| const score = subject.scoreValue; | ||
|
|
||
| if (score > highest) highest = score; | ||
|
|
||
| const weight = getSubjectWeight(subject.SubjectName); | ||
| totalWeightedScore += score * weight; | ||
| totalWeight += weight; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Using a for...of loop would improve readability while maintaining the performance benefits of a single pass. Additionally, replacing the if condition with highest = Math.max(highest, score) is more idiomatic and ensures consistent NaN handling. If any scoreValue is NaN, highest will correctly become NaN, matching the behavior of weightedAvg and the original implementation (which used Math.max(...scores)).
for (const subject of subjects) {
const score = subject.scoreValue;
highest = Math.max(highest, score);
const weight = getSubjectWeight(subject.SubjectName);
totalWeightedScore += score * weight;
totalWeight += weight;
}There was a problem hiding this comment.
Pull request overview
Optimizes the dashboard’s updateStatistics hot path by removing intermediate array creation and avoiding Math.max(...scores) (which can throw for very large arrays), while keeping statistics rendering in a single O(n) pass.
Changes:
- Replaced
subjects.map(...)+Math.max(...)+forEach(...)with a single indexedforloop. - Computes
highestand weighted average in one pass to reduce allocations and prevent spread-related runtime errors.
Comments suppressed due to low confidence (1)
frontend/dashboard.js:153
- This change is specifically meant to prevent
Math.max(...scores)from throwing with very large subject lists, but there’s no automated test covering the new statistics calculation or the “massive array” regression. Consider extracting the calculation into a small pure helper (e.g., returns{ highest, weightedAvg }) and adding a unit test that runs it on a large input to ensure it doesn’t throw and still returns correct values.
// 計算最高分與加權平均,合併迴圈以減少陣列分配與迭代,並避免 Math.max 展開大陣列時可能發生的 Stack Overflow
let highest = -Infinity;
let totalWeightedScore = 0;
let totalWeight = 0;
for (let i = 0; i < subjects.length; i++) {
const subject = subjects[i];
const score = subject.scoreValue;
if (score > highest) highest = score;
const weight = getSubjectWeight(subject.SubjectName);
totalWeightedScore += score * weight;
totalWeight += weight;
}
const weightedAvg = totalWeight > 0 ? totalWeightedScore / totalWeight : 0;
document.getElementById('avgScore').textContent = weightedAvg.toFixed(1);
document.getElementById('totalSubjects').textContent = subjects.length;
document.getElementById('highestScore').textContent = highest;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 計算最高分與加權平均,合併迴圈以減少陣列分配與迭代,並避免 Math.max 展開大陣列時可能發生的 Stack Overflow | ||
| let highest = -Infinity; | ||
| let totalWeightedScore = 0; | ||
| let totalWeight = 0; | ||
|
|
||
| subjects.forEach(subject => { | ||
| for (let i = 0; i < subjects.length; i++) { | ||
| const subject = subjects[i]; | ||
| const score = subject.scoreValue; | ||
|
|
||
| if (score > highest) highest = score; | ||
|
|
||
| const weight = getSubjectWeight(subject.SubjectName); | ||
| totalWeightedScore += score * weight; | ||
| totalWeight += weight; | ||
| }); | ||
| } |
There was a problem hiding this comment.
scoreValue can be NaN (see getNumericScore() fallback path), and the new loop can leave highest as -Infinity when all scores are non-finite, which then renders -Infinity in the UI. Also, a single NaN score will propagate totalWeightedScore to NaN and make weightedAvg.toFixed(1) show NaN. Consider skipping non-finite scores (and their weights) when computing highest/average, and rendering '--' (or 0) when no valid numeric scores exist.
💡 What: Combined
.map(),Math.max(), and.forEach()into a singleforloop insideupdateStatistics.🎯 Why: To reduce redundant loop iterations and avoid intermediate array allocations. Specifically, it prevents
RangeError: Maximum call stack size exceededthat occurs when using the spread operator (Math.max(...scores)) with potentially massive arrays.📊 Impact: Guarantees a single O(n) pass over the subject list, minimizing memory allocation/garbage collection overhead, and prevents application crashes on render due to stack size limits.
🔬 Measurement: Verify zero visual regression and improved execution logic within frontend Playwright testing; monitor runtime without throwing stack errors for massive arrays.
PR created automatically by Jules for task 6954969009140730277 started by @alvin000009238