-
Notifications
You must be signed in to change notification settings - Fork 510
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
Conversation
There was a problem hiding this 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?
done |
packages/runtime/src/routeGeneration/templates/express/expressTemplateService.ts
Outdated
Show resolved
Hide resolved
if( headers ){ | ||
Object.keys(headers).forEach((name: string) => { | ||
response.set(name, headers[name]); | ||
}); | ||
} |
There was a problem hiding this comment.
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 ...
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
packages/runtime/src/routeGeneration/templates/express/expressTemplateService.ts
Outdated
Show resolved
Hide resolved
if( headers ){ | ||
Object.keys(headers).forEach((name: string) => { | ||
response.set(name, headers[name]); | ||
}); | ||
} |
There was a problem hiding this comment.
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
#1692 (comment) |
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, 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 ( |
logically, In this situation, examine its existence is a logically wrong. Since you raise the clean code issue, |
done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about these changes.
I thought you are going to add implicitly type on header.
All Submissions:
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:
Potential Problems With The Approach
Test plan