Categories deep nesting #154
Open
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.
P1B: Starter Task: Refactoring PR
Use this pull request template to briefly answer the questions below in one to two sentences each.
Feel free to delete this text at the top after filling out the template.
1. Issue
Link to the associated GitHub issue:
#79
Full path to the refactored file:
public/src/admin/manage/categories.js
What do you think this file does?
(Your answer does not have to be 100% correct; give a reasonable, evidence‑based guess.)
This file purpose is to allow NodeBB's administrators to deal with the creation, organisation, and editing of categories that appear in NodeBB's homepage from the administrator panel.
What is the scope of your refactoring within that file?
(Name specific functions/blocks/regions touched.)
The only function that was touched was the itemDragDidEnd() function, as I removed the 5th level of nesting from it. Also, I did create a new helper function called updateCategoryToggleVisibility() directly above itemDragDidEnd on lines 203 - 208.
Which Qlty‑reported issue did you address?
(Name the rule/metric and include the BEFORE value; e.g., “Cognitive Complexity 18 in render()”.)
public/src/admin/manage/categories.js
232 Deeply nested control flow (level = 5)
2. Refactoring
How did the specific issue you chose impact the codebase’s maintainability?
The issue I chose allows future developers of NodeBB to easily understand how toggle visibility for categories with children works, as previously the logic was nested inside of 5 if statements, which made the code less interpretable. So, if someone wants to alter the logic for toggle visibility on categories, it would be much easier to do so given that you would just need to modify a single helper function.
What changes did you make to resolve the issue?
As mentioned before, the logic for the toggle button being visible on a category was nested in 5 conditionals, so to fix this I created a helper function called updateCategoryToggleVisibility() that handles updating the toggle visibility. The function takes two parameters, the category id and isVisible (boolean). The new helper function checks if the id of the parent of the category you are dragging has changed, and if it has, then you update the new parent category id's toggle to be visible by feeding updateCategoryToggleVisibility() the new parent category id and isVisible = true. Then, nested in this conditional is a check to see if the old parent category id has any children, and if it doesn't updateCategoryToggleVisibility() is fed the old parent category id and isVisible = false. Originally, there was a 5th conditional when checking if the old category id has any more children, but this helper function approach removes this deep nesting and simplifies the code.
How do your changes improve maintainability? Did you consider alternatives?
My changes improve the readability of the itemDragDidEnd() function, as it was confusing at first to interpret how the toggle visibility was being handles on categories that have children. Now it is clear that how the toggle visibility is handled, as the updateCategoryToggleVisibility() function describes what it does and the parameter values that are passed to it are easily interpreted. It also reduces code duplication as the same toggle logic appeared twice before my changes.
3. Validation
How did you trigger the refactored code path from the UI?
Attach a screenshot of the logs and UI demonstrating the trigger.
(If you refactored a public/src/ file (front-end related file), watch logging via DevTools (Ctrl+Shift+I to open and then navigate to the 'Console' tab). If you refactored a src/ file, watch logging via ./nodebb log. Include the relevant UI view. Temporary logs should be removed before final commit.)
(This image took place after I removed the blog category from being a child of announcements to being a stand alone category.)
Attach a screenshot of
qlty smells --no-snippets <full/path/to/file.js>showing fewer reported issues after the changes.(Screenshot of qty smell before refactoring)
(Screenshot of qlty after refactoring)