Skip to content

Conversation

@ryan-twang
Copy link
Owner

P1B: Starter Task: Refactoring PR

1. Issue

Please provide a link to the associated GitHub issue:

CMU-313/NodeBB#1

Link to the associated GitHub issue:

CMU-313/NodeBB#1

Full path to the refactored file:
src/user/index.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 provides many of the core user operations (e.g fetching users from Redis, mapping usernames/emails to uids, and status checks). My evidence-based guess stems from playing around trying to trigger console.log().

What is the scope of your refactoring within that file?
(Name specific functions/blocks/regions touched.)
I only changed User.getUsersFromSet (starting at line 75). I changed its default and argument handling from 4 to 3 while trying to keep behavior the same. I did not change any other functions/blocks/regions.

Which Qlty‑reported issue did you address?
(Name the rule/metric and include the BEFORE value; e.g., “Cognitive Complexity 18 in render()”.)
Qlty reported the issue as "Function with many parameters (count = 4)" for getUsersFromSet. After I refactored, the warning no longer shows

2. Refactoring

How did the specific issue you chose impact the codebase’s adaptability?
From my perspective, four sequential parameters seemed to make the function easy to misuse (e.g. argument order). And it makes it more difficult to extend safely without touching other callers.

What changes did you make to resolve the issue?
I reduced the parameters from 4 to 3 (set, uid, start) and set defined defaults (start=0, stop=-1). And I tried to preserve its functionality/behavior by reading an optional fourth argument through arguments[3]

How do your changes improve adaptability? Did you consider alternatives?
The new 3 parameter function is harder to misuse and still works with old calls. It improves readability and future edits without interrupting old calls. I considered adding a small helper class to hold start/stop, but ended up choosing the smallest change so nothing else needed updates.

3. Validation

How did you trigger the refactored code path from the UI?
I followed the writeup by adding console.log('Ryan Wang') then ran ./nodebb log. I was able to trigger the console.log() by visiting User -> Banned/Most Flags/Most Reputation, Groups -> Admin (members)/Global Moderators(members). Screen Shots of UI + triggered logs ('Ryan Wang') are posted below.

Attach a screenshot of the logs and UI demonstrating the trigger.
(Run ./nodebb log; include the relevant UI view. Temporary logs should be removed before final commit.)

Screenshot 2025-09-02 at 11 28 52 PM Screenshot 2025-09-02 at 11 28 34 PM Screenshot 2025-09-02 at 11 26 01 PM Screenshot 2025-09-02 at 11 25 48 PM Screenshot 2025-09-02 at 11 25 55 PM Screenshot 2025-09-02 at 11 26 01 PM Screenshot 2025-09-02 at 5 03 41 PM

Attach a screenshot of qlty smells --no-snippets <full/path/to/file.js> showing fewer reported issues after the changes.
Screenshot 2025-09-02 at 5 06 36 PM

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