Bind code optimization/formatting #23110
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Koenkk
reviewed
Jun 22, 2024
| .reduce((a, v) => a.concat(v)).filter((b) => b.target === data.group.zh); | ||
| const bindsToGroup: zh.Bind[] = []; | ||
|
|
||
| for (const device of this.zigbee.devices(false)) { |
There was a problem hiding this comment.
What's the benefit of writing it in for loops over the old code?
There was a problem hiding this comment.
It removes a lot of looping. Chained calls make it too easy to forget the depth of the looping 😅
┌─────────┬───────────┬─────────────┬────────────────────┬──────────┬──────────┐
│ (index) │ Task Name │ ops/sec │ Average Time (ns) │ Margin │ Samples │
├─────────┼───────────┼─────────────┼────────────────────┼──────────┼──────────┤
│ 0 │ 'Old' │ '417,144' │ 2397.2496359703237 │ '±0.36%' │ 2085724 │
│ 1 │ 'New' │ '3,984,551' │ 250.96924324733993 │ '±0.23%' │ 19922760 │
└─────────┴───────────┴─────────────┴────────────────────┴──────────┴──────────┘
Koenkk
reviewed
Jun 22, 2024
lib/extension/bind.ts
Outdated
| return false; | ||
| } | ||
|
|
||
| if (e.configuredReportings.some((c) => c.cluster.name === bind.cluster.name) && |
There was a problem hiding this comment.
Same here, in my opinion this is now harder to read, the variable names of the old code gave some clue what is happening here.
There was a problem hiding this comment.
Re-introduced variable names while still keeping the short-circuiting to avoid unnecessary looping on earlier check fail.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@Koenkk make sure to triple-check those refactored chained function calls. Tests seem a little light, and some of the logic has lots of depths, easy for coverage (or the dev 😅) to get fooled.
Also, if you see an easy way to fix the coverage, looks like it wasn't detected before.
Also: