Skip to content

Conversation

@ygonz174
Copy link

@ygonz174 ygonz174 commented Jan 23, 2026

P1B: Starter Task: Refactoring PR

1. Issue

Link to the associated GitHub issue:
https://github.com/CMU-313/NodeBB/issues/20#issue-3825901640

Full path to the refactored file:
/workspaces/NodeBB/src/user/blocks.js

What do you think this file does?
This file manages the interaction with multiple users by giving them the ability to block other users if they choose to do so.

What is the scope of your refactoring within that file?
(Name specific functions/blocks/regions touched.)
User.blocks.can

Which Qlty‑reported issue did you address?
(Name the rule/metric and include the BEFORE value; e.g., “Cognitive Complexity 18 in render()”.)
Function with many parameters (count = 4)

2. Refactoring

How did the specific issue you chose impact the codebase’s maintainability?
This specific issue impacted the maintainability by having to many redundant parameters. It is not good practice since it can cause mismatch orders, the caller can accidentally put it in the wrong order and in general its hard to read.

What changes did you make to resolve the issue?
To resolve this issue, instead of making it so that the caller changes, I captured all the arguments as one variable as an array so that it can be organized. I made it so that the array was a list and was able to parse each element out of it so that we don't have 4 arguments passed into the function, but instead just one.

How do your changes improve maintainability? Did you consider alternatives?
The changes I made allow the usage of more parameters If we ever want to increase the arguments in the function the caller can add more and it won't break anything, it makes it more adaptable to changes. As an alternative, I considered changing the caller but I was unsure how many calls there could be in all the files that could possibly be calling the function.

3. Validation

How did you trigger the refactored code path from the UI?
I was not able to do this, since I was having a gui error and could not display or use the block button.
Attach a screenshot of the logs and UI demonstrating the trigger.
Screenshot 2026-01-22 at 9 55 55 PM

(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.
qlty

NOTE: Regarding Test Coverage Report below
The drop in overall coverage is due to socket.io.js being commented out. This was necessary because, when included before and after the change, the file was causing the build and tests to fail across all categories in both instances.

I discussed this issue with Professor Hilton, and he instructed me to document the cause here and note that the relevant changes remain tested. Despite the overall decrease, 100% of the lines changed and added in this PR are covered.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 21273666177

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • 421 unchanged lines in 34 files lost coverage.
  • Overall coverage decreased (-1.4%) to 77.44%

Files with Coverage Reduction New Missed Lines %
src/activitypub/helpers.js 1 34.93%
src/api/users.js 1 82.01%
src/meta/logs.js 1 87.5%
src/middleware/csrf.js 1 76.0%
src/socket.io/admin/errors.js 1 75.0%
src/socket.io/admin/settings.js 1 93.33%
src/file.js 2 76.64%
src/groups/membership.js 2 96.2%
src/meta/themes.js 2 71.33%
src/posts/cache.js 2 77.27%
Totals Coverage Status
Change from base Build 21149476722: -1.4%
Covered Lines: 24941
Relevant Lines: 30329

💛 - 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