Refactor (src/user/data.js): reduce parameters for incrDecrUserFieldBy#41
Open
newmoooon wants to merge 4 commits intoCMU-313:mainfrom
Open
Refactor (src/user/data.js): reduce parameters for incrDecrUserFieldBy#41newmoooon wants to merge 4 commits intoCMU-313:mainfrom
newmoooon wants to merge 4 commits intoCMU-313:mainfrom
Conversation
Pull Request Test Coverage Report for Build 21154062026Details
💛 - Coveralls |
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:
#12
Full path to the refactored file:
src/user/data.js
What do you think this file does?
(Your answer does not have to be 100% correct; give a reasonable, evidence‑based guess.)
I think this file extracts and edits user data because there are functions to get the userField, hide data, modify data, and ensure required fields. The name of the file is data.js, so it is consistent that it manages data by ensuring that the fields are good and storing the user information.
What is the scope of your refactoring within that file?
(Name specific functions/blocks/regions touched.)
The function with the issue is "async function incrDecrUserFieldBy(uid, field, value, type)", which had 4 arguments (causing the issue). I removed the type argument. There are also two functions above it that call this function (they are User.incrementUserFieldBy and User.decrementUserFieldBy). All 3 of these functions are at the end of the file in data.js and are the only times incrDecrUserFieldBy is called.
Which Qlty‑reported issue did you address?
(Name the rule/metric and include the BEFORE value; e.g., “Cognitive Complexity 18 in render()”.)
The Qlty-reported issue I addressed was “Function with many parameters (count = 4)” in the incrDecrUserFieldBy function on line 408 in data.js.
2. Refactoring
How did the specific issue you chose impact the codebase’s maintainability?
The fourth parameter was simply unnecessary because type can be derived from value, which is already a parameter. It makes the codebase harder to maintain, because the value and its type could be inconsistent. For example, if the value is positive, but I want to change it to negative, then I must ensure that the type is changed as well, making it less maintainable and more prone to errors.
What changes did you make to resolve the issue?
I removed the type parameter from incrDecrUserFieldBy, which was unnecessary because the type (either increment or decrement) is dependent on the value, which is already a parameter. Therefore, in the function, I set type to increment if value is not negative, and decrement otherwise. Then I updated, User.incrementUserFieldBy and User.decrementUserFieldBy to call that function with either value (for increment) and -value (for decrement).
How do your changes improve maintainability? Did you consider alternatives?
My change reduce the unnecessary parameter and made the arguments independent from each other. We do not need to check that value and type are consistent. I did not consider other alternatives, because this method felt like the simplest way. I felt that there was no need to keep this parameter or add other helpers if I could simply remove it. This method also did not require changing anything else aside from the 3 functions mentioned, reducing the potential of introducing more errors.
3. Validation
How did you trigger the refactored code path from the UI?
To trigger the code, I opened NodeBB via localhost:4567. I opened Announcements and I selected New Topic. I filled in the topic title, post content, and tags and submitted the topic. Using ./nodebb log, I can confirm that the code was executed because it printed xinyue_wang.
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 includes the log where "xinyue_wang" was printed and a screenshot of the UI where I created the topic.

Attach a screenshot of

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