fix: Separate solution logic from comments.js #378
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @yammesicka - I've reviewed your changes and they look great!
General suggestions:
- Consider exploring more declarative approaches or DOM parsing libraries to optimize readability and performance in updateOpenedSpans.
- Investigate safer methods for HTML manipulation to mitigate potential security risks associated with direct innerHTML manipulation.
- Reevaluate the necessity of the 'load' event listener for script initialization to potentially enhance script execution time.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟡 Security: 1 issue found
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Docstrings: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
lms/static/solution.js
Outdated
| function updateOpenedSpans(currentSpans, line) { | ||
| /* Because we have each line wrapped in it's own span, we must close | ||
| * all the opened spans in this specific line and re-open them in the next | ||
| * line. This function help us to manage the state of open span tags. | ||
| */ | ||
| let isCatching = false; | ||
| let phrase = ''; | ||
| for (let i = 0; i < line.length; i += 1) { | ||
| const c = line[i]; | ||
| if (c === '>') { | ||
| isCatching = false; | ||
| phrase = `<${phrase}>`; | ||
| if (phrase === '</span>') { | ||
| currentSpans.pop(); | ||
| } else if (phrase.startsWith('<span')) { | ||
| currentSpans.push(phrase); | ||
| } | ||
| phrase = ''; | ||
| } else if (c === '<') { | ||
| isCatching = true; | ||
| } else if (isCatching) { | ||
| phrase += c; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion (code_refinement): Consider optimizing the updateOpenedSpans function for readability and performance.
The current implementation uses string concatenation and checks within a loop, which might be optimized for better readability and possibly performance by using a more declarative approach or leveraging existing DOM parsing libraries.
| phrase += c; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🚨 issue (security): Ensure proper handling of special characters in HTML.
The direct manipulation of innerHTML could lead to issues with special characters or potential security vulnerabilities. Consider using textContent and createElement for safer and more controlled manipulation.
| window.addEventListener('load', () => { | ||
| addLineSpansToPre(document.getElementsByTagName('code')); | ||
| }); |
There was a problem hiding this comment.
suggestion (performance): Review the necessity of the 'load' event listener for initializing the script.
If the script tag is placed at the end of the body, the DOM content would already be loaded, making the 'load' event listener redundant. This could improve the script's execution time.
| const wrappedLine = `<div class="line-container" data-line="${i + 1}"><span class="line-number" style="width: ${digits}em">${i + 1}</span> <span data-line="${i + 1}" class="line">${lineContent}</span></div>`; | ||
| return wrappedLine; |
There was a problem hiding this comment.
suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
| const wrappedLine = `<div class="line-container" data-line="${i + 1}"><span class="line-number" style="width: ${digits}em">${i + 1}</span> <span data-line="${i + 1}" class="line">${lineContent}</span></div>`; | |
| return wrappedLine; | |
| return `<div class="line-container" data-line="${i + 1}"><span class="line-number" style="width: ${digits}em">${i + 1}</span> <span data-line="${i + 1}" class="line">${lineContent}</span></div>`; | |
Explanation
Something that we often see in people's code is assigning to a result variableand then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #378 +/- ##
=======================================
Coverage 80.48% 80.48%
=======================================
Files 65 65
Lines 3049 3049
=======================================
Hits 2454 2454
Misses 595 595 ☔ View full report in Codecov by Sentry. |
No description provided.