Skip to content

Remove some O(n²) calls to reduce#147

Merged
skonves merged 1 commit intomainfrom
mapper-n2
Nov 30, 2025
Merged

Remove some O(n²) calls to reduce#147
skonves merged 1 commit intomainfrom
mapper-n2

Conversation

@skonves
Copy link
Member

@skonves skonves commented Nov 30, 2025

No description provided.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR aims to improve performance by replacing O(n²) reduce operations with O(n) for-loops in mapper functions. The key change is replacing the pattern Object.keys(obj).reduce((acc, key) => { ..., return { ...acc, [key]: value } }, {}) with direct object mutation using for-loops, which avoids creating a new object on each iteration.

Key changes:

  • Refactored pure map mapper functions to use for-loops instead of reduce with spread operators
  • Relocated type union definitions to the end of files (after functions)
  • Added TODO comments for remaining O(n²) code that wasn't refactored

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
packages/typescript-dtos/src/mapper-factory.ts Updated code generator for pure map mappers; added TODOs for mixed mappers and compact function that weren't fully refactored
packages/typescript-dtos/src/snapshot/server/v1/types.ts Moved union type declarations after function definitions
packages/typescript-dtos/src/snapshot/client/v1/types.ts Moved union type declarations after function definitions
packages/http-client/src/snapshot/zod/v1/dtos/mappers.ts Refactored pure map functions to use for-loops; mixed map functions still use reduce
packages/express/src/snapshot/zod/v1/types.ts Removed deprecation comment from GetGizmosParams.search property
packages/express/src/snapshot/zod/v1/dtos/mappers.ts Refactored pure map functions to use for-loops; mixed map functions still use reduce
Comments suppressed due to low confidence (2)

packages/typescript-dtos/src/mapper-factory.ts:251

  • The TODO comment indicates this code should be refactored to use a for-loop (like the change in buildPureMapMapper at lines 354-359), but the refactoring was not completed. This leaves the O(n²) performance issue unfixed for mixed type mappers.

The refactoring should be:

const result: ${this.buildTypeNames(type.name.value, mode).output} = __defined__;
for (const key of Object.keys(__rest__)) {
  const value = ${this.builder.buildValue(mapProperties.value.value, mode, `${paramName}[key]`, asType)};
  if (value !== undefined) result[key] = value;
}
return result;
      // TODO: remove O(n²) call to reduce
      yield ``;
      yield `return Object.keys(__rest__).reduce((acc, key) => {`;
      yield `const value = ${this.builder.buildValue(mapProperties.value.value, mode, `${paramName}[key]`, asType)};`;
      yield `return value === undefined ? acc : { ...acc, [key]: value };`;
      yield `}, __defined__ as ${this.buildTypeNames(type.name.value, mode).output});`;

packages/typescript-dtos/src/mapper-factory.ts:733

  • The TODO comment indicates this compact function should be refactored to remove the O(n²) call to reduce, but the refactoring was not completed. While this is a utility function that processes individual objects, the same performance improvement should be applied here for consistency.

The refactoring should be:

function compact<T extends object>(obj: T): T {
  const result = {} as T;
  for (const key of Object.keys(obj)) {
    const value = obj[key as keyof T];
    if (value !== undefined) {
      result[key as keyof T] = value;
    }
  }
  return result;
}
    // TODO: remove O(n²) call to reduce
    if (this._needsCompact) {
      yield `function compact<T extends object>(obj: T): T {
        return Object.keys(obj).reduce((acc, key) => {
          const value = obj[key as keyof T];
          if (value !== undefined) {
            acc[key as keyof T] = value;
          }
          return acc;
        }, {} as T);
      }`;
    }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

body?: ExhaustiveParamsBody;
};

export type GetGizmosParams = {
Copy link

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The @deprecated JSDoc comment was removed from the search property. This appears to be an unintentional change that's unrelated to the PR's stated purpose of removing O(n²) calls. The deprecation status should be preserved unless there's a deliberate reason to remove it. Other deprecation comments remain in the codebase (e.g., line 202 for Gizmo type, line 207 for size property).

Suggested change
export type GetGizmosParams = {
export type GetGizmosParams = {
/**
* @deprecated
*/

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@skonves skonves merged commit 18c22b9 into main Nov 30, 2025
12 checks passed
@skonves skonves deleted the mapper-n2 branch November 30, 2025 18:28
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