⚡ Bolt: [performance improvement] Optimize updateStatistics array iterations - #124
⚡ Bolt: [performance improvement] Optimize updateStatistics array iterations#124alvin000009238 wants to merge 1 commit into
Conversation
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.
Pull request overview
This PR optimizes the dashboard’s updateStatistics() hot path by reducing repeated iterations over the subjects array, aiming to lower GC pressure and improve runtime performance in the frontend.
Changes:
- Replaced
subjects.map(...)+Math.max(...)+forEach(...)with a single indexedforloop. - Computes both weighted average and highest score in one pass.
Comments suppressed due to low confidence (1)
frontend/dashboard.js:154
highestis initialized to-Infinityand only updated whenscoreValuecompares greater. If allsubject.scoreValuevalues areNaN/undefined(possible viagetNumericScore()returningNumber(fallbackValue)),highestwill remain-Infinityand be rendered to the UI. Consider tracking whether any finite score was seen (or checkingNumber.isFinite(highest)after the loop) and falling back to'--'(and optionally skipping non-finite scores in the weighted average too) to avoid displaying-Infinity/NaNto users.
// 計算加權平均與最高分
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.
There was a problem hiding this comment.
Code Review
This pull request refactors the updateStatistics function in frontend/dashboard.js to calculate both the highest score and the weighted average in a single loop, replacing the previous multi-pass approach. The review feedback suggests using a more idiomatic for...of loop and utilizing Math.max within the loop to correctly handle NaN values and maintain behavioral parity with the original implementation.
| 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.
While the manual for loop is efficient, using a for...of loop is more idiomatic in modern JavaScript and improves readability by removing the need for manual index management. Modern engines optimize for...of to perform nearly identically to a standard for loop for array iterations.
Additionally, consider using highest = Math.max(highest, score) to maintain behavioral parity with the original implementation regarding NaN values. The current if (score > highest) logic will ignore NaN scores, whereas the original Math.max(...scores) would result in NaN if any score was invalid. If all scores happen to be invalid, the current code would incorrectly display -Infinity in the UI.
for (const subject of subjects) {
const score = subject.scoreValue;
highest = Math.max(highest, score);
const weight = getSubjectWeight(subject.SubjectName);
totalWeightedScore += score * weight;
totalWeight += weight;
}
💡 What: Combined
.map(),Math.max(), and.forEach()calls into a singleforloop inupdateStatisticsinsidefrontend/dashboard.js.🎯 Why: Iterating over the array three times and creating intermediate arrays causes unnecessary GC overhead and overhead in time complexity.
📊 Impact: Reduces time complexity constants and prevents one unnecessary array allocation.
🔬 Measurement: Code runs faster and creates fewer intermediate array objects.
PR created automatically by Jules for task 13231556317909425004 started by @alvin000009238