P1B: Refactor (src/privileges/topics.js:170): Split complex binary expression into 3 separate consts for easier understanding #276
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
topics.js in privsTopics.canDelete into 3 consts so that the separate parts of the binary expression can be more easily understood.
from canDelete in topics.js.
commit due to new ordering of isAdminister leading to more failed tests.
order of the code.
Pull Request Test Coverage Report for Build 17603617301Details
💛 - Coveralls |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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
1. Issue
Please provide a link to the associated GitHub issue:
Link to the associated GitHub issue:
#272
Full path to the refactored file:
src/privileges/topics.js
What do you think this file does?
This file topics.js seems to check if the user has permissions to do different actions related to topics. There are checks of privileges of users using their user id that happen throughout this file. As for the specific canDelete function, I believe it is run to check if a user is allowed to delete a topic.
What is the scope of your refactoring within that file?
The return line of the function canDelete.
Which Qlty‑reported issue did you address?
Complex Binary Expression 170 in privsTopics.canDelete().
2. Refactoring
How did the specific issue you chose impact the codebase’s adaptability?
The specific issue that I chose impacted the codebase's adaptability because it was hard to interpret what was being returned by the function canDelete as it was a large binary expression.
What changes did you make to resolve the issue?
I split the central section of the binary expression into a const labeled "MiddleExpression" and then checked if the user is a moderator and stored that into a more interpretable const called "roleCheck" and returned allowedTo[0] && roleCheck.
How do your changes improve adaptability? Did you consider alternatives?
My changes improve adaptability by making the function canDelete easier to understand for someone reading the code, as the value being returned uses fewer binary operators, and the name of what's being checked is clearer.
3. Validation
How did you trigger the refactored code path from the UI?
I signed into a non-admin account and then posted a topic, then deleted it. This triggered my print into ./nodebb log.
Attach a screenshot of the logs and UI demonstrating the trigger.
Attach a screenshot of
qlty smells --no-snippets <full/path/to/file.js>showing fewer reported issues after the changes.