Skip to content

Conversation

@sssiiiiiii
Copy link

P1B: Starter Task: Refactoring PR

Use this pull request template to briefly answer the questions below in one to two sentences each.
Feel free to delete this text at the top after filling out the template.

You are not permitted to use generative AI services (e.g., ChatGPT) to compose the answers.
Any such use will be treated as a violation of academic integrity.

1. Issue

Please provide a link to the associated GitHub issue:

Link to the associated GitHub issue:
#169

Full path to the refactored file:
public/src/modules/share.js

What do you think this file does?
(Your answer does not have to be 100% correct; give a reasonable, evidence‑based guess.)
This file registers front-end share behavior for posts. When a user clicks the share dropdown and a social network icon is selected (Twitter, Facebook, WhatsApp, Telegram, LinkedIn), it opens a popup to share the post and fires a hook.

What is the scope of your refactoring within that file?
(Name specific functions/blocks/regions touched.)
Inside module.addShareHandlers, refactored the internal helper openShare and updated all calls for Twitter, Facebook, WhatsApp, Telegram, LinkedIn.

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) in openShare().

2. Refactoring

How did the specific issue you chose impact the codebase’s adaptability?
Passing 4 parameters (url, urlToPost, width, height) made the helper hard to read and easy to misuse (argument order mistakes etc). Adding new features would also require more positional params, impacting readability and flexibility.

What changes did you make to resolve the issue?
Rewrote openShare to accept a single options object, if opts isn’t an object, it falls back to the previous positional signature. Updated all internal call sites to pass an object (eg, { url, urlToPost, width, height }).

How do your changes improve adaptability? Did you consider alternatives?
Using an options object is clean and would improve readability, as well as making it easy for documentation, naming fields, making the defaults explicit and centralised. It makes it possible to scale for future fields a messy number of params. Alternatively I thought about keeping positional params but introducing a global constant.

3. Validation

How did you trigger the refactored code path from the UI?
Clicked into a post, then below the post there are three buttons, click share button then clicked Twitter.

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.)
Port crashed.

Attach a screenshot of qlty smells --no-snippets <full/path/to/file.js> showing fewer reported issues after the changes.
Screenshot 2025-09-05 at 2 49 19 AM

@coveralls
Copy link

Pull Request Test Coverage Report for Build 17485956171

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 78.529%

Totals Coverage Status
Change from base Build 17435863312: 0.0%
Covered Lines: 24692
Relevant Lines: 29606

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