Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: avoid useless create new empty object #1692

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

cesco69
Copy link

@cesco69 cesco69 commented Oct 2, 2024

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you written unit tests?
  • Have you written unit tests that cover the negative cases (i.e.: if bad data is submitted, does the library respond properly)?
  • This PR is associated with an existing issue?

Closing issues

Put closes #XXXX (where XXXX is the issue number) in your comment to auto-close the issue that your PR fixes.

If this is a new feature submission:

  • Has the issue had a maintainer respond to the issue and clarify that the feature is something that aligns with the goals and philosophy of the project?

Potential Problems With The Approach

Test plan

Copy link
Collaborator

@jackey8616 jackey8616 left a comment

Choose a reason for hiding this comment

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

I think there is more then one places use this syntax like hapi & koa template service.
Can you make it right in once?

@cesco69
Copy link
Author

cesco69 commented Oct 3, 2024

I think there is more then one places use this syntax like hapi & koa template service. Can you make it right in once?

done

Comment on lines +112 to +116
if( headers ){
Object.keys(headers).forEach((name: string) => {
response.set(name, headers[name]);
});
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This if statement is also can combine with or operator like:
Object.keys(headers || {}).forEach ...

Copy link
Author

Choose a reason for hiding this comment

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

I want avoid unecessary creation of new empty object

Copy link
Author

@cesco69 cesco69 left a comment

Choose a reason for hiding this comment

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

done

Comment on lines +112 to +116
if( headers ){
Object.keys(headers).forEach((name: string) => {
response.set(name, headers[name]);
});
}
Copy link
Author

Choose a reason for hiding this comment

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

I want avoid unecessary creation of new empty object

@jackey8616
Copy link
Collaborator

#1692 (comment)
Can I have reason?

@cesco69
Copy link
Author

cesco69 commented Oct 3, 2024

#1692 (comment) Can I have reason?

This version:

if (headers) {
  Object.keys(headers).forEach((name: string) => {
    response.set(name, headers[name]);
  });
}

is more performant than:

Object.keys(headers || {}).forEach((name: string) => { 
  response.set(name, headers[name]);
});

for the following reasons:

Unnecessary Operations: In the second version, Object.keys(headers || {}) is always called, even when headers is null or undefined. If headers is falsy, || {} creates an empty object ({}), and then Object.keys processes it. This results in a redundant operation when headers is falsy, as there are no keys to iterate over.

In contrast, the first version avoids calling Object.keys altogether if headers is falsy, saving the overhead of creating an empty object and calling Object.keys.

Control Flow Simplicity: The first version has a clear conditional check (if (headers)), and only when headers is truthy does the code proceed to iterate through the keys. This makes it easier for the JavaScript engine to optimize the code because there's no need to run operations when they are unnecessary.

Reduced Memory Usage: In the second version, when headers is falsy, the code creates a new empty object ({}) that only serves to satisfy the Object.keys call. Although the memory impact of creating an empty object is small, it is still a waste when compared to the first version, which simply skips this step when not needed.

@jackey8616
Copy link
Collaborator

#1692 (comment) Can I have reason?

This version:

if (headers) {
  Object.keys(headers).forEach((name: string) => {
    response.set(name, headers[name]);
  });
}

is more performant than:

Object.keys(headers || {}).forEach((name: string) => { 
  response.set(name, headers[name]);
});

for the following reasons:

Unnecessary Operations: In the second version, Object.keys(headers || {}) is always called, even when headers is null or undefined. If headers is falsy, || {} creates an empty object ({}), and then Object.keys processes it. This results in a redundant operation when headers is falsy, as there are no keys to iterate over.

In contrast, the first version avoids calling Object.keys altogether if headers is falsy, saving the overhead of creating an empty object and calling Object.keys.

Control Flow Simplicity: The first version has a clear conditional check (if (headers)), and only when headers is truthy does the code proceed to iterate through the keys. This makes it easier for the JavaScript engine to optimize the code because there's no need to run operations when they are unnecessary.

Reduced Memory Usage: In the second version, when headers is falsy, the code creates a new empty object ({}) that only serves to satisfy the Object.keys call. Although the memory impact of creating an empty object is small, it is still a waste when compared to the first version, which simply skips this step when not needed.

logically, header should not include the undefined or null value.
It should be represents empty with {} empty object.

In this situation, examine its existence is a logically wrong.
the why it design like version 1 is a historical reason, at the time of refactoring it I just simply have no time to think deeper but just trying to make it easier when reading, that's why I didn't wrap with anoterh if statement.

Since you raise the clean code issue,
Let take the "more clean" solutions that we deal with the root cause: caller need to correctly pass the parameter without any incorrect type.
If we do so, we can save a if statement, stay clean.

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