Refactor (public/src/client/infinitescroll.js:40): reduce complexity in onScroll() #139
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What the file does
The file implements the client-side infinite scrolling behaviour in NodeBB. It keeps a track of the window scroll events, calculates how far the user has scrolled relative to the container and then triggers a callback to load more content.
The scope of the refactoring
Refactored was done only on the onScroll() function. No changes were made to the behaviours of the function. I created a few helper functions to keep everything clean and easy to read.
Qlty Smell Addressed
Function with high complexity (count = 11): onScroll in public/src/client/infinitescroll.js from line 40. The following was the output of the Qlty-reported issue
public/src/client/infinitescroll.js 40 Function with high complexity (count = 11): onScrollREFACTORING
in onScroll, the high complexity (count = 11) reduced adaptability since the logic was condensed into one function. This made it harder to understand and modify the code.
I refactored onScroll() by adding helper functions that did one job each.
- skipScroll() was made to have all the early-exit conditions
- getMetrics() was created to compute currentScrollTop, viewportHeight and scrollPercent
- getDirection(currentScrollTop) was created to compute scroll direction in relation to previousScrollTop
- getTrigger(metrics) contained trigger rules for when to call the callback
The changes made improved adaptability. The multiple responsibilities condensed into the onScroll() function were converted to a function each for easier reading. An alternative I considered was to make changes in the existing function but I was running into the same problem of the responsibilities being condensed into one function.
VALIDATION
I ran NodBB locally, opened the site in the browser, created a few posts myself by creating an account to trigger the infinite scroll function, opened the Console from the DevTools, scrolled down the posts page to trigger the infinite scrolling. I inserted a console.log('Sai Sankhe') statement which was shown as belows:
qlty smells after changes
