Skip to content

Refactor (src/user/data.js): reduce parameters for incrDecrUserFieldBy#41

Open
newmoooon wants to merge 4 commits intoCMU-313:mainfrom
newmoooon:refactor-incrDecrUserFieldBy
Open

Refactor (src/user/data.js): reduce parameters for incrDecrUserFieldBy#41
newmoooon wants to merge 4 commits intoCMU-313:mainfrom
newmoooon:refactor-incrDecrUserFieldBy

Conversation

@newmoooon
Copy link

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.
image

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

@coveralls
Copy link

Pull Request Test Coverage Report for Build 21154062026

Details

  • 6 of 6 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 78.841%

Totals Coverage Status
Change from base Build 21149476722: 0.0%
Covered Lines: 25361
Relevant Lines: 30331

💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments