Skip to content

refactor(SaveInterface): extract duplicated strings to constants#5190

Merged
walterbender merged 1 commit intosugarlabs:masterfrom
WillyEverGreen:refactor/saveinterface-duplicated-strings
Feb 21, 2026
Merged

refactor(SaveInterface): extract duplicated strings to constants#5190
walterbender merged 1 commit intosugarlabs:masterfrom
WillyEverGreen:refactor/saveinterface-duplicated-strings

Conversation

@WillyEverGreen
Copy link
Contributor

Overview

This PR addresses multiple Code Quality issues identified by SonarJS and ESLint regarding duplicated string literals in js/SaveInterface.js.

Changes Made

  • Extracted repeated string literals into constants:
    • STR_MY_PROJECT for "My Project"
    • STR_SHOW for "Show"
    • STR_HIDE for "Hide"
    • STR_SHOWHIDE for the "showhide" element id
  • Refactored htmlSaveTemplate to use these constants.
  • Resolved the SonarJS rule: sonarjs/no-duplicate-string.

Benefits

  • Maintainability: Common UI strings are now defined in one place, simplifying future updates and localization.
  • Clarity: Reduces repetition and improves readability.
  • Quality: Clears static analysis warnings, allowing focus on higher-priority security issues.

Verification

  • Ran npm run lint and confirmed sonarjs/no-duplicate-string warnings are resolved for js/SaveInterface.js.
  • Ran npx prettier to ensure consistent formatting.

@github-actions
Copy link
Contributor

✅ All Jest tests passed! This PR is ready to merge.

@walterbender
Copy link
Member

I wonder if it would make more sense to put the translated strings into the constants so we don't have to repeatedly translate them?

@github-actions
Copy link
Contributor

✅ All Jest tests passed! This PR is ready to merge.

@WillyEverGreen
Copy link
Contributor Author

I wonder if it would make more sense to put the translated strings into the constants so we don't have to repeatedly translate them?

Thanks, got to know something new today and you were right, I've reflected upon your suggestion.

@walterbender
Copy link
Member

I am not crazy about obscuring these strings:
const STR_SHOWHIDE = "showhide";
const STR_BTN_TEXT_AZ = 'showHideButton.textContent = "';

It makes the code less clear, IMHO.

But I am fine with the strings exposed to the interface.

@WillyEverGreen WillyEverGreen force-pushed the refactor/saveinterface-duplicated-strings branch from 2f0b0db to 0e6d39f Compare January 26, 2026 11:59
@github-actions
Copy link
Contributor

✅ All Jest tests passed! This PR is ready to merge.

@WillyEverGreen WillyEverGreen force-pushed the refactor/saveinterface-duplicated-strings branch from 0e6d39f to b1d210b Compare January 26, 2026 12:29
@github-actions
Copy link
Contributor

✅ All Jest tests passed! This PR is ready to merge.

@WillyEverGreen
Copy link
Contributor Author

@walterbender I've updated the PR. I've extracted the user-facing strings into constants (STR_MY_PROJECT, STR_SHOW, STR_HIDE) but kept internal identifiers like "showhide" inline for clarity as you suggested.

@vanshika2720
Copy link
Contributor

@WillyEverGreen Since these constants call _() at file load time, are we certain translations are fully initialized before this script executes?

I just want to confirm this won’t cause issues if language initialization happens later or if language switching is supported at runtime.

@walterbender
Copy link
Member

Should be OK

@walterbender walterbender merged commit f5199e5 into sugarlabs:master Feb 21, 2026
6 checks passed
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.

3 participants