⚡ Bolt: Combine multiple O(n) array iterations into a single loop - #131
⚡ Bolt: Combine multiple O(n) array iterations into a single loop#131alvin000009238 wants to merge 1 commit into
Conversation
…ons into a single loop Replaced sequential array operations `.map()`, `Math.max()`, and `.forEach()` in `updateStatistics` with a single `for` loop to reduce array allocation overhead and minimize GC pressure. 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. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request optimizes the updateStatistics function in frontend/dashboard.js by consolidating the calculation of the highest score and weighted average into a single for loop, which improves performance by reducing iterations. Additionally, the build asset hashes for CSS and JS files in public/index.html have been updated. A review comment suggests adding a check for NaN values within the loop to prevent them from corrupting the weighted average calculation.
| for (let i = 0; i < subjects.length; i++) { | ||
| const subject = subjects[i]; | ||
| const score = subject.scoreValue; | ||
| const weight = getSubjectWeight(subject.SubjectName); | ||
|
|
||
| if (score > highest) highest = score; | ||
|
|
||
| totalWeightedScore += score * weight; | ||
| totalWeight += weight; | ||
| }); | ||
| } |
There was a problem hiding this comment.
The current loop is susceptible to NaN poisoning. If any subject.scoreValue is NaN, the totalWeightedScore (and consequently the weightedAvg) will become NaN. While the highest calculation correctly ignores NaN values (since NaN > -Infinity is false), the average calculation does not. It's safer to skip NaN values to ensure the statistics remain valid even if some data is missing or malformed.
| for (let i = 0; i < subjects.length; i++) { | |
| const subject = subjects[i]; | |
| const score = subject.scoreValue; | |
| const weight = getSubjectWeight(subject.SubjectName); | |
| if (score > highest) highest = score; | |
| totalWeightedScore += score * weight; | |
| totalWeight += weight; | |
| }); | |
| } | |
| for (let i = 0; i < subjects.length; i++) { | |
| const subject = subjects[i]; | |
| const score = subject.scoreValue; | |
| if (Number.isNaN(score)) continue; | |
| const weight = getSubjectWeight(subject.SubjectName); | |
| if (score > highest) highest = score; | |
| totalWeightedScore += score * weight; | |
| totalWeight += weight; | |
| } |
There was a problem hiding this comment.
Pull request overview
Refactors the dashboard statistics calculation to compute highest score and weighted average in a single pass, reducing redundant array iterations and intermediate allocations.
Changes:
- Refactored
updateStatistics()to computehighestandweightedAvgin one loop. - Updated
public/index.htmlto reference new hashed Vite build outputs (/dist/main-*.cssand/dist/main-*.js).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| public/index.html | Updates referenced hashed build assets for the frontend bundle. |
| frontend/dashboard.js | Combines multiple O(n) passes into a single loop for statistics calculation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <script src="https://challenges.cloudflare.com/turnstile/v0/api.js?render=explicit" async defer></script> | ||
| <script src="/theme-init.js"></script> | ||
| <link rel="stylesheet" href="/dist/main-bt8E1vXG.css" id="vite-css"> | ||
| <link rel="stylesheet" href="/dist/main-D2sAJccc.css" id="vite-css"> |
There was a problem hiding this comment.
vite.config.js has emptyOutDir: true, so a new build removes old hashed assets. This PR updates /dist/main-*.css and /dist/main-*.js in public/index.html, but other static pages under public/ (e.g. public/privacy.html) still reference older /dist/main-*.css hashes and will 404 / lose styling after deploy. Consider extending scripts/inject-hash.js to update all public/*.html asset references (or standardize the stylesheet tag so it can be rewritten everywhere).
| <link rel="stylesheet" href="/dist/main-D2sAJccc.css" id="vite-css"> | |
| <link rel="stylesheet" href="/dist/main.css" id="vite-css"> |
| // 計算最高分與加權平均，將多次 O(n) 迴圈合併為單次，減少 GC 開銷並提升效能 | ||
| let highest = -Infinity; | ||
| let totalWeightedScore = 0; | ||
| let totalWeight = 0; | ||
|
|
||
| subjects.forEach(subject => { | ||
| for (let i = 0; i < subjects.length; i++) { | ||
| const subject = subjects[i]; |
There was a problem hiding this comment.
There are tests for frontend/dashboard.js, but the refactor changes observable behavior of updateStatistics (highest score + weighted average calculation). Please add a unit test (e.g., via JSDOM like tests/frontend/theme.test.js) that asserts avgScore, highestScore, and totalSubjects output for a small subject list to prevent regressions.
| document.getElementById('avgScore').textContent = weightedAvg.toFixed(1); | ||
| document.getElementById('totalSubjects').textContent = subjects.length; | ||
| document.getElementById('highestScore').textContent = highest; | ||
| document.getElementById('highestScore').textContent = highest === -Infinity ? '--' : highest; | ||
| } |
There was a problem hiding this comment.
Since the function returns early when subjects.length is 0, highest should never remain -Infinity for normal numeric inputs; the highest === -Infinity ? '--' : highest branch is effectively dead code and can hide unexpected NaN/invalid scores. Consider initializing highest from the first element (and iterating from index 1) so you can drop the sentinel/conditional and keep the display logic consistent.
💡 What: Refactored
updateStatisticsto compute the highest score and weighted average in a single pass instead of using.map(),Math.max(), and.forEach().🎯 Why: Iterating over the array three times and creating intermediate arrays is inefficient and triggers unnecessary garbage collection, especially on mobile devices with constrained memory or in high-frequency rendering scenarios.
📊 Impact: Reduces array iteration from O(3n) to O(n) and avoids allocating an intermediate
scoresarray. This saves memory and prevents theoreticalMaximum call stack size exceedederrors if the dataset were massive.🔬 Measurement: Run
node --test tests/frontend/dashboard.test.jsto confirm the logic remains valid, and observe faster execution times in browser profilers during heavy initial dashboard renders.PR created automatically by Jules for task 16223660044411969809 started by @alvin000009238