Skip to content

#8576 Refactor: Array-mutating methods should not be used misleadingly - #8577

Merged
AlexeyGirin merged 2 commits into
masterfrom
copilot/refactor-array-mutating-methods
Nov 19, 2025
Merged

AlexeyGirin merged 2 commits into
masterfrom
copilot/refactor-array-mutating-methods

Conversation

Copilot AI commented Nov 18, 2025 •

Copy link
Copy Markdown
Contributor

How the feature works? / How did you fix the issue?

SonarQube flagged two instances where reverse() was used misleadingly—the method mutates the array and returns it, obscuring whether mutation is intentional.

Changes:

  • Line 3253: selectedPiece.reverse().forEach() → [...selectedPiece].reverse().forEach()
  • Line 3349: senseNode.monomers.reverse().forEach() → [...senseNode.monomers].reverse().forEach()

The spread syntax creates a shallow copy before reversing, eliminating side effects and clarifying intent.

Example:

// Before: mutates original array
selectedPiece.reverse().forEach((node) => { /* ... */ });

// After: operates on copy
[...selectedPiece].reverse().forEach((node) => { /* ... */ });

Check list

  • unit-tests written
  • e2e-tests written
  • documentation updated
  • PR name follows the pattern #1234 – issue name
  • branch name doesn't contain '#'
  • PR is linked with the issue
  • base branch (master or release/xx) is correct
  • task status changed to "Code review"
  • reviewers are notified about the pull request
Original prompt

This section details on the original issue you should resolve

<issue_title>Refactor: Array-mutating methods should not be used misleadingly</issue_title>
<issue_description>Problem:
Move this array "reverse" operation to a separate statement or replace it with "toReversed".

Why is this an issue?
In JavaScript, some Array methods do not mutate the existing array that the method was called on, but instead return a new array. Other methods mutate the array, and their return value differs depending on the method.
reverse and sort are mutating methods and, in addition, return the altered version. This rule raises an issue when the return values of these methods are assigned, which could lead maintainers to overlook the fact that the original array has been modified.

const reversed = a.reverse(); // Noncompliant: mutating method, no need to assign return value
const sorted = b.sort(); // Noncompliant: mutating method, no need to assign return value

Remove the assignment, so that the intent of mutating the original array is clear.

a.reverse();
b.sort();

Or use non-mutating alternatives toReversed and toSorted.

const reversed = a.toReversed();
const sorted = b.toSorted();

Alternatively, change a mutating method into a non-mutating alternative using the spread syntax (…​).

const reversed = [...a].reverse();
const sorted = [...b].sort();

Or slice() to create a copy first.

const reversed = a.slice().reverse();
const sorted = b.slice().sort();

Problem locations:
packages/ketcher-core/src/domain/entities/DrawingEntitiesManager.ts:3253
packages/ketcher-core/src/domain/entities/DrawingEntitiesManager.ts:3349</issue_description>

Comments on the Issue (you are @copilot in this section)


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Co-authored-by: AlexeyGirin <26869421+AlexeyGirin@users.noreply.github.com>
Copilot AI changed the title [WIP] Refactor misleading usage of array mutating methods Refactor: Replace misleading array mutation with spread syntax Nov 18, 2025
Copilot AI requested a review from AlexeyGirin November 18, 2025 17:16
@AlexeyGirin
AlexeyGirin marked this pull request as ready for review November 18, 2025 17:23
@AlexeyGirin AlexeyGirin changed the title Refactor: Replace misleading array mutation with spread syntax #8576 Refactor: Array-mutating methods should not be used misleadingly Nov 18, 2025
@AlexeyGirin
AlexeyGirin merged commit 2c7d392 into master Nov 19, 2025
33 of 37 checks passed
@AlexeyGirin
AlexeyGirin deleted the copilot/refactor-array-mutating-methods branch November 19, 2025 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor: Array-mutating methods should not be used misleadingly

3 participants