PR: Refactor (src/messaging/notifications.js:105): Complex binary expression #188#189
Open
asrnCMU wants to merge 2 commits intoCMU-313:mainfrom
Open
PR: Refactor (src/messaging/notifications.js:105): Complex binary expression #188#189asrnCMU wants to merge 2 commits intoCMU-313:mainfrom
asrnCMU wants to merge 2 commits intoCMU-313:mainfrom
Conversation
…ule (by reducing binary expression complexity)
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Link to the associated GitHub issue:
#188
Full path to the refactored file:
src/messaging/notifications.js
What do you think this file does?
Contains the logic for triggering notifications. It checks various conditions, such as user settings or whether user is a member, in various settings (groups, chats etc.) and triggers a notification accordingly.
What is the scope of your refactoring within that file?
Only changed the binary expression contained in the async function sendNotification. This expression handled the logic of whether a notification should be triggered based on chat room and user settings.
Which Qlty‑reported issue did you address?
Complex binary expression (line 105) in src/messaging/notifications.js
2. Refactoring
How did the specific issue you chose impact the codebase’s maintainability?
The original complex binary expression combined several checks (some containing multiple steps) into a large, highly nested expression. With the reasoning or logic behind each check not being commented and the complexity of the expression, it is hard to read and understand the code, especially for future maintenance such as extending the conditions or debugging.
What changes did you make to resolve the issue?
I split the complex binary expression into smaller, more digestable components. Each check has an appropriate const name, and ones with several steps have consts as intermediate combination of terms.
How do your changes improve maintainability? Did you consider alternatives?
The multistage breakdown improves ease of debugging as the purpose of each check is evident and it is much easier to split and individually test each check (or identify which one is failing), as opposed to the less granular combined expression. I tried refactoring by returning after certain checks, to further break the code into sections. However this caused more issues (too many returns in a function), and so I stopped going down this route.
3. Validation
How did you trigger the refactored code path from the UI?
I created new users, then created a chat between them. Sending a message from one user to another present in the chat satisfies the criteria (binary expression) to trigger a notification, and therefore the code path was clearly shown to be triggered in the console log.
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.)
Attach a screenshot of

qlty smells --no-snippets <full/path/to/file.js>showing fewer reported issues after the changes.