Readability improvement for SchedulerMinHeap. #20557
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit f9cc0ba:
|
| const first = heap[0]; | ||
| return first === undefined ? null : first; | ||
| if (heap.length === 0) return null; | ||
| return heap[0]; |
There was a problem hiding this comment.
As previous code states, heap is possibly undefined and your code tries to read length on it what may lead to a runtime error.
There was a problem hiding this comment.
There seems no usage that passes a undefined to it. If there is, it seems that the line of code const first = heap[0]; will lead to an error too.
There was a problem hiding this comment.
Yes, that's true and I was thinking about something else in fact. Your code will fail if the first element of heap is undefined. Remember, arrays can have empty elements. So you could write something like
if (typeof heap[0] === 'undefined') return null;.
There was a problem hiding this comment.
You're right but we operate the heap by pop and push, what you worry about will not happen.
| let minIndex = index; | ||
| if (rightIndex < length) { | ||
| // As a heap is a complete binary tree, node with right child must have the left one. | ||
| minIndex = smaller(heap, minIndex, smaller(heap, leftIndex, rightIndex)); |
There was a problem hiding this comment.
For me adding more abstraction here isn't necessarily good for readability, often it is much better to read bare logic even if it takes a few more lines.
There was a problem hiding this comment.
For me here are too many statements for describing How to do. Maybe to describe What to do is better?
|
Thanks for the PR, but we generally don't take PRs that make things "cleaner". They very rarely help and usually tend to introduce subtle bugs. In general, we prefer verbose low-level code that's inlined instead of outlined helpers even when they're more legible. |
|
@gaearon Got it. Thanks for your explanation. |
Refactor the implementation of min-heap to make the logic clearer.