⚡ Bolt: Combine array iterations in updateStatistics - #125
Conversation
…tistics Replaced `.map()`, `Math.max()`, and `.forEach()` with a single O(n) `for` loop in `updateStatistics` in `frontend/dashboard.js`. This eliminates intermediate array allocations, prevents potential "Maximum call stack size exceeded" errors with large arrays in `Math.max()`, and reduces GC 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 in frontend/dashboard.js by consolidating multiple array iterations into a single loop to calculate the highest score and weighted average. Feedback suggests adopting a more idiomatic for...of loop and refining the code comments and formatting for improved readability.
| // Bolt: Combine O(n) iterations to eliminate intermediate array and Math.max | ||
| 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.
The traditional for loop with manual index management is less idiomatic in modern JavaScript when the index is not explicitly needed. Switching to a for...of loop improves readability and maintainability. Additionally, removing the "Bolt:" prefix from the comment and cleaning up the extra empty lines within the loop will result in cleaner code.
// Combine O(n) iterations to eliminate intermediate array and Math.max
for (const subject of subjects) {
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.
Pull request overview
This PR optimizes updateStatistics() in the frontend dashboard by merging multiple array passes into a single loop to reduce allocations and avoid potential Math.max(...largeArray) issues.
Changes:
- Replaced
subjects.map(...)+Math.max(...scores)+forEachwith a single indexedforloop. - Tracks
highestwhile accumulating weighted sums in one pass.
Comments suppressed due to low confidence (2)
frontend/dashboard.js:153
higheststarts at-Infinityand is only updated whenscore > highest. Ifsubject.scoreValuecan beNaN(which is possible viagetNumericScore()fallback),highestcan remain-Infinityand be rendered to the UI. Consider guarding withNumber.isFinite(score)(skip or coerce invalid scores) and ensure the displayed highest score falls back to'--'/0rather than-Infinitywhen no valid scores exist.
let highest = -Infinity;
let totalWeightedScore = 0;
let totalWeight = 0;
// Bolt: Combine O(n) iterations to eliminate intermediate array and Math.max
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;
frontend/dashboard.js:151
updateStatistics()logic is changed here but isn’t covered by existingtests/frontend/dashboard.test.js(only pure helpers likegetNumericScore/shortenNameare tested). Consider extracting the statistics calculation into a pure helper (e.g., returning{ highest, weightedAvg }) and adding a small unit test for edge cases (empty subjects, NaN scores, etc.).
let highest = -Infinity;
let totalWeightedScore = 0;
let totalWeight = 0;
// Bolt: Combine O(n) iterations to eliminate intermediate array and Math.max
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);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let totalWeight = 0; | ||
|
|
||
| subjects.forEach(subject => { | ||
| // Bolt: Combine O(n) iterations to eliminate intermediate array and Math.max |
There was a problem hiding this comment.
The inline comment includes "Bolt:", which looks like an internal tool/reference rather than describing runtime behavior. Please reword it to a code-focused explanation (or remove it) so future readers understand the intent without needing tooling context.
| // Bolt: Combine O(n) iterations to eliminate intermediate array and Math.max | |
| // 使用單一 O(n) 迴圈同時計算最高分與加權總分,避免建立中間陣列與額外一次 Math.max 掃描 |
| // Bolt: Combine O(n) iterations to eliminate intermediate array and Math.max | ||
| for (let i = 0; i < subjects.length; i++) { | ||
| const subject = subjects[i]; |
There was a problem hiding this comment.
PR description suggests verifying with pnpm run test, but this repo’s package.json defines tests under npm run test (no pnpm usage found). Please update the PR description/instructions to match the actual tooling to avoid confusion for reviewers/CI triage.
💡 What: Replaced chained array methods (
.map,Math.max,.forEach) with a singleforloop inupdateStatisticsinsidefrontend/dashboard.js.🎯 Why: The original code iterated over the
subjectsarray three times and created an intermediate array, increasing garbage collection churn. Using the spread operator withMath.maxon potentially large arrays is an anti-pattern that can cause stack overflow. Consolidating into one O(n) loop improves execution speed and memory efficiency.📊 Impact: Reduces redundant iterations from O(3n) to O(n), eliminates intermediate array allocations, and prevents potential
RangeErrorwith large datasets.🔬 Measurement: Verify tests pass using
pnpm run testand that the application renders the dashboard accurately.PR created automatically by Jules for task 16987334601984261413 started by @alvin000009238