MaxHeap implementation in JavaScript
Here is MaxHeap implementation in pure JavaScript:
/**
* We initialize index 0 because, calculations for finding
* children of a node and the parent of a node is easier
* parent = [ k/2 ], children: [ k*2, k*2+1 ]
*/
class Heap {
constructor() {
this.queue = [0];
};
/**
* Bottom-up reheapify (swim) : If the heap order is violated because a node's key
* becomes larger than that node's parents key, then we can make progress
* toward fixing the violation by exchanging the node with its parent
*
* @param {Number} k
*/
swim(k) {
while ( k>1 && this.less(k, Heap.flr(k/2)) ) {
this.exch(k, Heap.flr(k/2) ) ;
k = Heap.flr(k/2);
}
};
/**
* Top-down heapify (sink) : If the heap order is violated because a node's key becomes
* smaller than one or both of that node's children's keys, then we can make progress
* toward fixing the violation by exchanging the node with the larger of its two children.
*
* @param {Number} k
*/
sink(k) {
while ( 2*k <= this.n() ) {
const c1 = 2*k;
const c2 = 2*k + 1;
const j = this.more(c1,c2)?c2:c1;
if (this.more(k, j) ) {
this.exch(k, j);
} else {
break;
}
k = j;
}
};
/**
*
* @param {Number} i
* @param {Number} j
*/
less(i,j) {
return this.queue[i]>this.queue[j];
};
/**
*
* @param {Number} i
* @param {Number} j
*/
more(i,j) {
return this.queue[i]<this.queue[j];
};
/**
*
* @param {Number} i
* @param {Number} j
*/
exch(i, j) {
const tmp = this.queue[i];
this.queue[i] = this.queue[j];
this.queue[j] = tmp;
};
/**
* Inserts element into the internal queue
* @param {Number} k
*/
insert(k) {
this.queue.push(k);
this.swim(this.n());
};
/**
* Returns max element in the internal queue
*/
max() {
if (!this.isEmpty() ) {
return this.queue[1];
}
return false;
};
/**
* Deletes and returns max element from the internal queue
*/
delMax() {
if (!this.isEmpty()) {
const mx = this.queue[1];
this.queue[1] = this.queue.pop();
if (!this.isEmpty()) {
this.sink(1);
}
return mx;
};
return false;
};
/**
* Checks for emptyness (here we consider the queue empty if the length of the queue is 1)
*/
isEmpty() {
return this.queue.length === 1;
};
/**
* Returns last element index in the internal queue
*/
n() {
return this.queue.length-1;
};
/**
* in JS dividing odd number by even number gives fraction not integer,
* we need to get rid of this part
* @param {Number} i
*/
static flr(i) {
return Math.floor(i);
}
}
Is the implementation correct? What are the shortcomings that can be fixed?
javascript array heap
add a comment |
Here is MaxHeap implementation in pure JavaScript:
/**
* We initialize index 0 because, calculations for finding
* children of a node and the parent of a node is easier
* parent = [ k/2 ], children: [ k*2, k*2+1 ]
*/
class Heap {
constructor() {
this.queue = [0];
};
/**
* Bottom-up reheapify (swim) : If the heap order is violated because a node's key
* becomes larger than that node's parents key, then we can make progress
* toward fixing the violation by exchanging the node with its parent
*
* @param {Number} k
*/
swim(k) {
while ( k>1 && this.less(k, Heap.flr(k/2)) ) {
this.exch(k, Heap.flr(k/2) ) ;
k = Heap.flr(k/2);
}
};
/**
* Top-down heapify (sink) : If the heap order is violated because a node's key becomes
* smaller than one or both of that node's children's keys, then we can make progress
* toward fixing the violation by exchanging the node with the larger of its two children.
*
* @param {Number} k
*/
sink(k) {
while ( 2*k <= this.n() ) {
const c1 = 2*k;
const c2 = 2*k + 1;
const j = this.more(c1,c2)?c2:c1;
if (this.more(k, j) ) {
this.exch(k, j);
} else {
break;
}
k = j;
}
};
/**
*
* @param {Number} i
* @param {Number} j
*/
less(i,j) {
return this.queue[i]>this.queue[j];
};
/**
*
* @param {Number} i
* @param {Number} j
*/
more(i,j) {
return this.queue[i]<this.queue[j];
};
/**
*
* @param {Number} i
* @param {Number} j
*/
exch(i, j) {
const tmp = this.queue[i];
this.queue[i] = this.queue[j];
this.queue[j] = tmp;
};
/**
* Inserts element into the internal queue
* @param {Number} k
*/
insert(k) {
this.queue.push(k);
this.swim(this.n());
};
/**
* Returns max element in the internal queue
*/
max() {
if (!this.isEmpty() ) {
return this.queue[1];
}
return false;
};
/**
* Deletes and returns max element from the internal queue
*/
delMax() {
if (!this.isEmpty()) {
const mx = this.queue[1];
this.queue[1] = this.queue.pop();
if (!this.isEmpty()) {
this.sink(1);
}
return mx;
};
return false;
};
/**
* Checks for emptyness (here we consider the queue empty if the length of the queue is 1)
*/
isEmpty() {
return this.queue.length === 1;
};
/**
* Returns last element index in the internal queue
*/
n() {
return this.queue.length-1;
};
/**
* in JS dividing odd number by even number gives fraction not integer,
* we need to get rid of this part
* @param {Number} i
*/
static flr(i) {
return Math.floor(i);
}
}
Is the implementation correct? What are the shortcomings that can be fixed?
javascript array heap
add a comment |
Here is MaxHeap implementation in pure JavaScript:
/**
* We initialize index 0 because, calculations for finding
* children of a node and the parent of a node is easier
* parent = [ k/2 ], children: [ k*2, k*2+1 ]
*/
class Heap {
constructor() {
this.queue = [0];
};
/**
* Bottom-up reheapify (swim) : If the heap order is violated because a node's key
* becomes larger than that node's parents key, then we can make progress
* toward fixing the violation by exchanging the node with its parent
*
* @param {Number} k
*/
swim(k) {
while ( k>1 && this.less(k, Heap.flr(k/2)) ) {
this.exch(k, Heap.flr(k/2) ) ;
k = Heap.flr(k/2);
}
};
/**
* Top-down heapify (sink) : If the heap order is violated because a node's key becomes
* smaller than one or both of that node's children's keys, then we can make progress
* toward fixing the violation by exchanging the node with the larger of its two children.
*
* @param {Number} k
*/
sink(k) {
while ( 2*k <= this.n() ) {
const c1 = 2*k;
const c2 = 2*k + 1;
const j = this.more(c1,c2)?c2:c1;
if (this.more(k, j) ) {
this.exch(k, j);
} else {
break;
}
k = j;
}
};
/**
*
* @param {Number} i
* @param {Number} j
*/
less(i,j) {
return this.queue[i]>this.queue[j];
};
/**
*
* @param {Number} i
* @param {Number} j
*/
more(i,j) {
return this.queue[i]<this.queue[j];
};
/**
*
* @param {Number} i
* @param {Number} j
*/
exch(i, j) {
const tmp = this.queue[i];
this.queue[i] = this.queue[j];
this.queue[j] = tmp;
};
/**
* Inserts element into the internal queue
* @param {Number} k
*/
insert(k) {
this.queue.push(k);
this.swim(this.n());
};
/**
* Returns max element in the internal queue
*/
max() {
if (!this.isEmpty() ) {
return this.queue[1];
}
return false;
};
/**
* Deletes and returns max element from the internal queue
*/
delMax() {
if (!this.isEmpty()) {
const mx = this.queue[1];
this.queue[1] = this.queue.pop();
if (!this.isEmpty()) {
this.sink(1);
}
return mx;
};
return false;
};
/**
* Checks for emptyness (here we consider the queue empty if the length of the queue is 1)
*/
isEmpty() {
return this.queue.length === 1;
};
/**
* Returns last element index in the internal queue
*/
n() {
return this.queue.length-1;
};
/**
* in JS dividing odd number by even number gives fraction not integer,
* we need to get rid of this part
* @param {Number} i
*/
static flr(i) {
return Math.floor(i);
}
}
Is the implementation correct? What are the shortcomings that can be fixed?
javascript array heap
Here is MaxHeap implementation in pure JavaScript:
/**
* We initialize index 0 because, calculations for finding
* children of a node and the parent of a node is easier
* parent = [ k/2 ], children: [ k*2, k*2+1 ]
*/
class Heap {
constructor() {
this.queue = [0];
};
/**
* Bottom-up reheapify (swim) : If the heap order is violated because a node's key
* becomes larger than that node's parents key, then we can make progress
* toward fixing the violation by exchanging the node with its parent
*
* @param {Number} k
*/
swim(k) {
while ( k>1 && this.less(k, Heap.flr(k/2)) ) {
this.exch(k, Heap.flr(k/2) ) ;
k = Heap.flr(k/2);
}
};
/**
* Top-down heapify (sink) : If the heap order is violated because a node's key becomes
* smaller than one or both of that node's children's keys, then we can make progress
* toward fixing the violation by exchanging the node with the larger of its two children.
*
* @param {Number} k
*/
sink(k) {
while ( 2*k <= this.n() ) {
const c1 = 2*k;
const c2 = 2*k + 1;
const j = this.more(c1,c2)?c2:c1;
if (this.more(k, j) ) {
this.exch(k, j);
} else {
break;
}
k = j;
}
};
/**
*
* @param {Number} i
* @param {Number} j
*/
less(i,j) {
return this.queue[i]>this.queue[j];
};
/**
*
* @param {Number} i
* @param {Number} j
*/
more(i,j) {
return this.queue[i]<this.queue[j];
};
/**
*
* @param {Number} i
* @param {Number} j
*/
exch(i, j) {
const tmp = this.queue[i];
this.queue[i] = this.queue[j];
this.queue[j] = tmp;
};
/**
* Inserts element into the internal queue
* @param {Number} k
*/
insert(k) {
this.queue.push(k);
this.swim(this.n());
};
/**
* Returns max element in the internal queue
*/
max() {
if (!this.isEmpty() ) {
return this.queue[1];
}
return false;
};
/**
* Deletes and returns max element from the internal queue
*/
delMax() {
if (!this.isEmpty()) {
const mx = this.queue[1];
this.queue[1] = this.queue.pop();
if (!this.isEmpty()) {
this.sink(1);
}
return mx;
};
return false;
};
/**
* Checks for emptyness (here we consider the queue empty if the length of the queue is 1)
*/
isEmpty() {
return this.queue.length === 1;
};
/**
* Returns last element index in the internal queue
*/
n() {
return this.queue.length-1;
};
/**
* in JS dividing odd number by even number gives fraction not integer,
* we need to get rid of this part
* @param {Number} i
*/
static flr(i) {
return Math.floor(i);
}
}
Is the implementation correct? What are the shortcomings that can be fixed?
javascript array heap
javascript array heap
asked Dec 16 at 16:00
Humoyun
1405
1405
add a comment |
add a comment |
2 Answers
2
active
oldest
votes
Bug
There's a bug in delMax
:
const mx = this.queue[1];
this.queue[1] = this.queue.pop();
if (!this.isEmpty()) {
this.sink(1);
}
return mx;
What will happen when the heap has 1 element? The code will:
- Replace the first element with itself (the last element)
- It doesn't become empty
In other words, the last element cannot be deleted.
Avoid hacky solutions
This code happens to work, but it's hacky:
while ( 2*k <= this.n() ) {
const c1 = 2*k;
const c2 = 2*k + 1;
const j = this.more(c1,c2)?c2:c1;
What's wrong with it? When 2*k == this.n()
, then c2
will point past the end of the queue, and more
will compare queue[c1]
with undefined
, and always return false
. The code works, but it would be better to not rely on numeric comparisons with undefined
.
Look for generalization opportunities
It's a good idea to use a function to decide the ordering of elements, similar to what you did with less
and more
. This could open the opportunity to make the implementation work with arbitrary ordering.
The current implementation doesn't make it easy to pass in these functions to customize the behavior. It would be good to make that possible. And it would be even better to make it work with a single function, let's say less
.
If you do that, then you should rename some methods, since max
and delMax
won't make sense for a min-heap ordering. You could rename these to peek
and pop
.
Hide implementation details
The class exposes many methods that are unnecessary for users:
swim
, sink
, less
, more
, exch
, flr
.
These are implementation details, and it would be best to hide them.
Drop the unnecessary trick
This trick:
/**
* We initialize index 0 because, calculations for finding
* children of a node and the parent of a node is easier
* parent = [ k/2 ], children: [ k*2, k*2+1 ]
*/
Looks completely unnecessary... I don't see a reason whatsoever why the dummy first element helps. You can eliminate it, and the implementation will be simpler and clearer.
Dividing by 2
I'm not sure if this is recommended in JavaScript world,
but instead of Math.floor(x / 2)
,
x >> 1
feels cleaner.
Naming
Some method names could be improved:
exch
->swap
n
->size
insert
->add
flr
->floor
, or instead of always calling it withflr(k / 2)
you could name itchildIndex
and call it withk
, letting it do the necessary math
flr
uses parameteri
. The namei
is best in simple counting loops.
1
Thank you very much for allocating time to write an excellent review of my implementation, I will refactor it based on your suggestions.
– Humoyun
Dec 16 at 23:56
add a comment |
Protect state
There is a fundamental overriding principle in programing that if correctly followed ensures that your code will work. Encapsulate and protect the objects state so that you can trust its integrity.
You have exposed the objects state yet provide no means to ensure that the state is maintained in such a way that your code will run as expected.
The following actions are all possible and all make the whole object unusable. You have no systematic way to determine if the state of heap is safe to use. Yet you continue to try and perform actions on a broken state, and will eventually throw an unknown error
const heap = new Heap();
heap.insert("ABC");
heap.queue.pop();
heap.queue.push(-100);
heap.queue.length = 0;
heap.queue = "monkey bread";
heap.exch(0,1000);
A good object means that there is NO WAY to corrupt the state of the object. That if the state cannot be maintained a predefined known list of errors will be thrown.
techniques to protect state
- Use closure to hide data that is not relevant to the user of the object.
- Use setters to ensure only valid values can be set.
- Use getters and setters to create read only and write only values.
- Avoid the
this
token as it can be hijacked and means any code that uses the token can corrupt state.
Performance
After making state trustable you must address performance. I am often down voted for putting too much emphasis on performance, yet when a new app or framework comes out the reviews always make a point off performance. Poor performance is an app killer.
Performance starts at the very lowest level and is a mind set as much as a style. You should always be thinking, is there a faster way. You should also test and know what style are performant as in JavaScript it is not always evident.
Inline is faster than function calls. It is a balancing act between readability and performance, but when readability is marginal always go for the inline option.
You have
while(2*k <= this.n())
but could bewhile(2 * k < queue.length)
Note queue should not be exposed.
Don't repeat calculations
In swim you repeat the same calculation 3 times
Heap.flr(k/2)
. It should be calculated once and stored as a variable. You do the same in other functions.
Don't pass known values
You call
sink
andswim
with argumentk
, which are always the same value (either1
orqueue.length - 1
). There is no need to pass a known value.
Integers are faster than doubles
Javascript does not have a definable integer type, but internally to take advantage of hardware performance Javascript
Number
can be a variety of types from uint32 to double. All bitwise operators use uint32 and will coerce doubles to uint32. Integers can be up to 2 times faster than doubles (more depending on the platform)
You have
Heap.flr(k/2)
that will convert odd values ofk
to a double, then floor it inflr
which converts it back to integer. Also you require two allocations to the call stack just to complete the operation.k >> 2
does the same thing, avoids the intermediate double and requires no use of the call stack.
Learn binary math and how to use bitwise operators in javascript as they provide significant performance benefits when working with integers. Note that Javascript integers are signed 32 bit values.
Warning about destructuring
Though you have not used destructuring, it is nowadays common to perform a swap using destructing. eg
[queue[a], queue[b]] = [queue[b], queue[a]]
and as a syntax it is very nice, but it is also currently extremely slow (About 10 times slower). You can get the same performance doing{const swap = [queue[b], queue[a]]; const swapped = [swap[1], swap[0]]; queue[a] = swapped[0]; queue[b] = swap[1]}
to give a hint at how it's performed under the hood.
I am sure this problem will be addressed soon and performance will follow standard styles, but for now be wary of destructuring in terms of performance.
Consistency
Try to be consistent with JavaScript. If you access an array item outside the range you get undefined
. However in your code you return false
It makes more sense to return undefined
when the queue is empty.
The rewrite
The rewrite addresses some more performance issues. The swim function can perform a lot of swaps, while in reality you are only looking for a place to put a new value. Rather than add the new value to the queue, locate the new position and use splice to insert it. This save a lot of data movements. Thus swim
is passed the value to swim up and be inserted
sink
can also be improved by starting at 0, same with the queue
, there is no need to hold an unused item at the start.
Removing the first item from queue
means testing if there are items becomes if(queue.length)
If the queue
is empty then undefined is returned. This eliminates the need to add conditional code to test for empty.
As sink
is only called to shift a value from the queue, it may as well shift that value as well simplifying the delMax
function now called shift
The code is much safer this way as it can not be misused, and performs a lot faster. It also reduces the source code size by half thus making it much easier to understand.
function Heap() {
const queue = ;
function sinkAndShift() {
var k = 0, k2 = 1;
while (k2 < queue.length) {
if (queue[k] < queue[k2]) {
const tmp = queue[k]; // don't use destructuring.
queue[k] = queue[k2];
queue[k2] = tmp;
} else { break }
k2 = (k = k2) * 2;
}
return queue.shift();
}
function swim(val) {
var k = queue.length - 1;
while (k > 0 && val > queue[k]) { k >>= 1 }
queue.splice(k, 0, val);
}
return Object.freeze({
set value(k) {
if (isNaN(k)) { throw new RangeError("Must be a number '" + k + "'") }
swim(Number(k));
},
shift() { return sinkAndShift() },
get max() { return queue[0] },
get length() { return queue.length },
});
}
// usage
const heap = new Heap();
// or
//const heap = new Heap;
// or
//const heap = Heap();
heap.value = 100; // same as insert
heap.value = 101; // adds second value
heap.value = 102;
heap.value = 103;
console.log(heap.length); // outputs 4;
console.log(heap.max); // outputs 103;
console.log(heap.shift()); // outputs 103;
console.log(heap.length); // outputs 3;
console.log(heap.shift()); // outputs 102;
console.log(heap.shift()); // outputs 101;
console.log(heap.shift()); // outputs 100;
console.log(heap.shift()); // outputs undefined;
heap.value = "Monkey bread"; // will throw a range error
Thank you very much for your efforts
– Humoyun
Dec 17 at 19:38
add a comment |
Your Answer
StackExchange.ifUsing("editor", function () {
return StackExchange.using("mathjaxEditing", function () {
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
});
});
}, "mathjax-editing");
StackExchange.ifUsing("editor", function () {
StackExchange.using("externalEditor", function () {
StackExchange.using("snippets", function () {
StackExchange.snippets.init();
});
});
}, "code-snippets");
StackExchange.ready(function() {
var channelOptions = {
tags: "".split(" "),
id: "196"
};
initTagRenderer("".split(" "), "".split(" "), channelOptions);
StackExchange.using("externalEditor", function() {
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled) {
StackExchange.using("snippets", function() {
createEditor();
});
}
else {
createEditor();
}
});
function createEditor() {
StackExchange.prepareEditor({
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader: {
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
},
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
});
}
});
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f209769%2fmaxheap-implementation-in-javascript%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
2 Answers
2
active
oldest
votes
2 Answers
2
active
oldest
votes
active
oldest
votes
active
oldest
votes
Bug
There's a bug in delMax
:
const mx = this.queue[1];
this.queue[1] = this.queue.pop();
if (!this.isEmpty()) {
this.sink(1);
}
return mx;
What will happen when the heap has 1 element? The code will:
- Replace the first element with itself (the last element)
- It doesn't become empty
In other words, the last element cannot be deleted.
Avoid hacky solutions
This code happens to work, but it's hacky:
while ( 2*k <= this.n() ) {
const c1 = 2*k;
const c2 = 2*k + 1;
const j = this.more(c1,c2)?c2:c1;
What's wrong with it? When 2*k == this.n()
, then c2
will point past the end of the queue, and more
will compare queue[c1]
with undefined
, and always return false
. The code works, but it would be better to not rely on numeric comparisons with undefined
.
Look for generalization opportunities
It's a good idea to use a function to decide the ordering of elements, similar to what you did with less
and more
. This could open the opportunity to make the implementation work with arbitrary ordering.
The current implementation doesn't make it easy to pass in these functions to customize the behavior. It would be good to make that possible. And it would be even better to make it work with a single function, let's say less
.
If you do that, then you should rename some methods, since max
and delMax
won't make sense for a min-heap ordering. You could rename these to peek
and pop
.
Hide implementation details
The class exposes many methods that are unnecessary for users:
swim
, sink
, less
, more
, exch
, flr
.
These are implementation details, and it would be best to hide them.
Drop the unnecessary trick
This trick:
/**
* We initialize index 0 because, calculations for finding
* children of a node and the parent of a node is easier
* parent = [ k/2 ], children: [ k*2, k*2+1 ]
*/
Looks completely unnecessary... I don't see a reason whatsoever why the dummy first element helps. You can eliminate it, and the implementation will be simpler and clearer.
Dividing by 2
I'm not sure if this is recommended in JavaScript world,
but instead of Math.floor(x / 2)
,
x >> 1
feels cleaner.
Naming
Some method names could be improved:
exch
->swap
n
->size
insert
->add
flr
->floor
, or instead of always calling it withflr(k / 2)
you could name itchildIndex
and call it withk
, letting it do the necessary math
flr
uses parameteri
. The namei
is best in simple counting loops.
1
Thank you very much for allocating time to write an excellent review of my implementation, I will refactor it based on your suggestions.
– Humoyun
Dec 16 at 23:56
add a comment |
Bug
There's a bug in delMax
:
const mx = this.queue[1];
this.queue[1] = this.queue.pop();
if (!this.isEmpty()) {
this.sink(1);
}
return mx;
What will happen when the heap has 1 element? The code will:
- Replace the first element with itself (the last element)
- It doesn't become empty
In other words, the last element cannot be deleted.
Avoid hacky solutions
This code happens to work, but it's hacky:
while ( 2*k <= this.n() ) {
const c1 = 2*k;
const c2 = 2*k + 1;
const j = this.more(c1,c2)?c2:c1;
What's wrong with it? When 2*k == this.n()
, then c2
will point past the end of the queue, and more
will compare queue[c1]
with undefined
, and always return false
. The code works, but it would be better to not rely on numeric comparisons with undefined
.
Look for generalization opportunities
It's a good idea to use a function to decide the ordering of elements, similar to what you did with less
and more
. This could open the opportunity to make the implementation work with arbitrary ordering.
The current implementation doesn't make it easy to pass in these functions to customize the behavior. It would be good to make that possible. And it would be even better to make it work with a single function, let's say less
.
If you do that, then you should rename some methods, since max
and delMax
won't make sense for a min-heap ordering. You could rename these to peek
and pop
.
Hide implementation details
The class exposes many methods that are unnecessary for users:
swim
, sink
, less
, more
, exch
, flr
.
These are implementation details, and it would be best to hide them.
Drop the unnecessary trick
This trick:
/**
* We initialize index 0 because, calculations for finding
* children of a node and the parent of a node is easier
* parent = [ k/2 ], children: [ k*2, k*2+1 ]
*/
Looks completely unnecessary... I don't see a reason whatsoever why the dummy first element helps. You can eliminate it, and the implementation will be simpler and clearer.
Dividing by 2
I'm not sure if this is recommended in JavaScript world,
but instead of Math.floor(x / 2)
,
x >> 1
feels cleaner.
Naming
Some method names could be improved:
exch
->swap
n
->size
insert
->add
flr
->floor
, or instead of always calling it withflr(k / 2)
you could name itchildIndex
and call it withk
, letting it do the necessary math
flr
uses parameteri
. The namei
is best in simple counting loops.
1
Thank you very much for allocating time to write an excellent review of my implementation, I will refactor it based on your suggestions.
– Humoyun
Dec 16 at 23:56
add a comment |
Bug
There's a bug in delMax
:
const mx = this.queue[1];
this.queue[1] = this.queue.pop();
if (!this.isEmpty()) {
this.sink(1);
}
return mx;
What will happen when the heap has 1 element? The code will:
- Replace the first element with itself (the last element)
- It doesn't become empty
In other words, the last element cannot be deleted.
Avoid hacky solutions
This code happens to work, but it's hacky:
while ( 2*k <= this.n() ) {
const c1 = 2*k;
const c2 = 2*k + 1;
const j = this.more(c1,c2)?c2:c1;
What's wrong with it? When 2*k == this.n()
, then c2
will point past the end of the queue, and more
will compare queue[c1]
with undefined
, and always return false
. The code works, but it would be better to not rely on numeric comparisons with undefined
.
Look for generalization opportunities
It's a good idea to use a function to decide the ordering of elements, similar to what you did with less
and more
. This could open the opportunity to make the implementation work with arbitrary ordering.
The current implementation doesn't make it easy to pass in these functions to customize the behavior. It would be good to make that possible. And it would be even better to make it work with a single function, let's say less
.
If you do that, then you should rename some methods, since max
and delMax
won't make sense for a min-heap ordering. You could rename these to peek
and pop
.
Hide implementation details
The class exposes many methods that are unnecessary for users:
swim
, sink
, less
, more
, exch
, flr
.
These are implementation details, and it would be best to hide them.
Drop the unnecessary trick
This trick:
/**
* We initialize index 0 because, calculations for finding
* children of a node and the parent of a node is easier
* parent = [ k/2 ], children: [ k*2, k*2+1 ]
*/
Looks completely unnecessary... I don't see a reason whatsoever why the dummy first element helps. You can eliminate it, and the implementation will be simpler and clearer.
Dividing by 2
I'm not sure if this is recommended in JavaScript world,
but instead of Math.floor(x / 2)
,
x >> 1
feels cleaner.
Naming
Some method names could be improved:
exch
->swap
n
->size
insert
->add
flr
->floor
, or instead of always calling it withflr(k / 2)
you could name itchildIndex
and call it withk
, letting it do the necessary math
flr
uses parameteri
. The namei
is best in simple counting loops.
Bug
There's a bug in delMax
:
const mx = this.queue[1];
this.queue[1] = this.queue.pop();
if (!this.isEmpty()) {
this.sink(1);
}
return mx;
What will happen when the heap has 1 element? The code will:
- Replace the first element with itself (the last element)
- It doesn't become empty
In other words, the last element cannot be deleted.
Avoid hacky solutions
This code happens to work, but it's hacky:
while ( 2*k <= this.n() ) {
const c1 = 2*k;
const c2 = 2*k + 1;
const j = this.more(c1,c2)?c2:c1;
What's wrong with it? When 2*k == this.n()
, then c2
will point past the end of the queue, and more
will compare queue[c1]
with undefined
, and always return false
. The code works, but it would be better to not rely on numeric comparisons with undefined
.
Look for generalization opportunities
It's a good idea to use a function to decide the ordering of elements, similar to what you did with less
and more
. This could open the opportunity to make the implementation work with arbitrary ordering.
The current implementation doesn't make it easy to pass in these functions to customize the behavior. It would be good to make that possible. And it would be even better to make it work with a single function, let's say less
.
If you do that, then you should rename some methods, since max
and delMax
won't make sense for a min-heap ordering. You could rename these to peek
and pop
.
Hide implementation details
The class exposes many methods that are unnecessary for users:
swim
, sink
, less
, more
, exch
, flr
.
These are implementation details, and it would be best to hide them.
Drop the unnecessary trick
This trick:
/**
* We initialize index 0 because, calculations for finding
* children of a node and the parent of a node is easier
* parent = [ k/2 ], children: [ k*2, k*2+1 ]
*/
Looks completely unnecessary... I don't see a reason whatsoever why the dummy first element helps. You can eliminate it, and the implementation will be simpler and clearer.
Dividing by 2
I'm not sure if this is recommended in JavaScript world,
but instead of Math.floor(x / 2)
,
x >> 1
feels cleaner.
Naming
Some method names could be improved:
exch
->swap
n
->size
insert
->add
flr
->floor
, or instead of always calling it withflr(k / 2)
you could name itchildIndex
and call it withk
, letting it do the necessary math
flr
uses parameteri
. The namei
is best in simple counting loops.
edited Dec 16 at 23:35
answered Dec 16 at 22:03
janos
97.1k12124350
97.1k12124350
1
Thank you very much for allocating time to write an excellent review of my implementation, I will refactor it based on your suggestions.
– Humoyun
Dec 16 at 23:56
add a comment |
1
Thank you very much for allocating time to write an excellent review of my implementation, I will refactor it based on your suggestions.
– Humoyun
Dec 16 at 23:56
1
1
Thank you very much for allocating time to write an excellent review of my implementation, I will refactor it based on your suggestions.
– Humoyun
Dec 16 at 23:56
Thank you very much for allocating time to write an excellent review of my implementation, I will refactor it based on your suggestions.
– Humoyun
Dec 16 at 23:56
add a comment |
Protect state
There is a fundamental overriding principle in programing that if correctly followed ensures that your code will work. Encapsulate and protect the objects state so that you can trust its integrity.
You have exposed the objects state yet provide no means to ensure that the state is maintained in such a way that your code will run as expected.
The following actions are all possible and all make the whole object unusable. You have no systematic way to determine if the state of heap is safe to use. Yet you continue to try and perform actions on a broken state, and will eventually throw an unknown error
const heap = new Heap();
heap.insert("ABC");
heap.queue.pop();
heap.queue.push(-100);
heap.queue.length = 0;
heap.queue = "monkey bread";
heap.exch(0,1000);
A good object means that there is NO WAY to corrupt the state of the object. That if the state cannot be maintained a predefined known list of errors will be thrown.
techniques to protect state
- Use closure to hide data that is not relevant to the user of the object.
- Use setters to ensure only valid values can be set.
- Use getters and setters to create read only and write only values.
- Avoid the
this
token as it can be hijacked and means any code that uses the token can corrupt state.
Performance
After making state trustable you must address performance. I am often down voted for putting too much emphasis on performance, yet when a new app or framework comes out the reviews always make a point off performance. Poor performance is an app killer.
Performance starts at the very lowest level and is a mind set as much as a style. You should always be thinking, is there a faster way. You should also test and know what style are performant as in JavaScript it is not always evident.
Inline is faster than function calls. It is a balancing act between readability and performance, but when readability is marginal always go for the inline option.
You have
while(2*k <= this.n())
but could bewhile(2 * k < queue.length)
Note queue should not be exposed.
Don't repeat calculations
In swim you repeat the same calculation 3 times
Heap.flr(k/2)
. It should be calculated once and stored as a variable. You do the same in other functions.
Don't pass known values
You call
sink
andswim
with argumentk
, which are always the same value (either1
orqueue.length - 1
). There is no need to pass a known value.
Integers are faster than doubles
Javascript does not have a definable integer type, but internally to take advantage of hardware performance Javascript
Number
can be a variety of types from uint32 to double. All bitwise operators use uint32 and will coerce doubles to uint32. Integers can be up to 2 times faster than doubles (more depending on the platform)
You have
Heap.flr(k/2)
that will convert odd values ofk
to a double, then floor it inflr
which converts it back to integer. Also you require two allocations to the call stack just to complete the operation.k >> 2
does the same thing, avoids the intermediate double and requires no use of the call stack.
Learn binary math and how to use bitwise operators in javascript as they provide significant performance benefits when working with integers. Note that Javascript integers are signed 32 bit values.
Warning about destructuring
Though you have not used destructuring, it is nowadays common to perform a swap using destructing. eg
[queue[a], queue[b]] = [queue[b], queue[a]]
and as a syntax it is very nice, but it is also currently extremely slow (About 10 times slower). You can get the same performance doing{const swap = [queue[b], queue[a]]; const swapped = [swap[1], swap[0]]; queue[a] = swapped[0]; queue[b] = swap[1]}
to give a hint at how it's performed under the hood.
I am sure this problem will be addressed soon and performance will follow standard styles, but for now be wary of destructuring in terms of performance.
Consistency
Try to be consistent with JavaScript. If you access an array item outside the range you get undefined
. However in your code you return false
It makes more sense to return undefined
when the queue is empty.
The rewrite
The rewrite addresses some more performance issues. The swim function can perform a lot of swaps, while in reality you are only looking for a place to put a new value. Rather than add the new value to the queue, locate the new position and use splice to insert it. This save a lot of data movements. Thus swim
is passed the value to swim up and be inserted
sink
can also be improved by starting at 0, same with the queue
, there is no need to hold an unused item at the start.
Removing the first item from queue
means testing if there are items becomes if(queue.length)
If the queue
is empty then undefined is returned. This eliminates the need to add conditional code to test for empty.
As sink
is only called to shift a value from the queue, it may as well shift that value as well simplifying the delMax
function now called shift
The code is much safer this way as it can not be misused, and performs a lot faster. It also reduces the source code size by half thus making it much easier to understand.
function Heap() {
const queue = ;
function sinkAndShift() {
var k = 0, k2 = 1;
while (k2 < queue.length) {
if (queue[k] < queue[k2]) {
const tmp = queue[k]; // don't use destructuring.
queue[k] = queue[k2];
queue[k2] = tmp;
} else { break }
k2 = (k = k2) * 2;
}
return queue.shift();
}
function swim(val) {
var k = queue.length - 1;
while (k > 0 && val > queue[k]) { k >>= 1 }
queue.splice(k, 0, val);
}
return Object.freeze({
set value(k) {
if (isNaN(k)) { throw new RangeError("Must be a number '" + k + "'") }
swim(Number(k));
},
shift() { return sinkAndShift() },
get max() { return queue[0] },
get length() { return queue.length },
});
}
// usage
const heap = new Heap();
// or
//const heap = new Heap;
// or
//const heap = Heap();
heap.value = 100; // same as insert
heap.value = 101; // adds second value
heap.value = 102;
heap.value = 103;
console.log(heap.length); // outputs 4;
console.log(heap.max); // outputs 103;
console.log(heap.shift()); // outputs 103;
console.log(heap.length); // outputs 3;
console.log(heap.shift()); // outputs 102;
console.log(heap.shift()); // outputs 101;
console.log(heap.shift()); // outputs 100;
console.log(heap.shift()); // outputs undefined;
heap.value = "Monkey bread"; // will throw a range error
Thank you very much for your efforts
– Humoyun
Dec 17 at 19:38
add a comment |
Protect state
There is a fundamental overriding principle in programing that if correctly followed ensures that your code will work. Encapsulate and protect the objects state so that you can trust its integrity.
You have exposed the objects state yet provide no means to ensure that the state is maintained in such a way that your code will run as expected.
The following actions are all possible and all make the whole object unusable. You have no systematic way to determine if the state of heap is safe to use. Yet you continue to try and perform actions on a broken state, and will eventually throw an unknown error
const heap = new Heap();
heap.insert("ABC");
heap.queue.pop();
heap.queue.push(-100);
heap.queue.length = 0;
heap.queue = "monkey bread";
heap.exch(0,1000);
A good object means that there is NO WAY to corrupt the state of the object. That if the state cannot be maintained a predefined known list of errors will be thrown.
techniques to protect state
- Use closure to hide data that is not relevant to the user of the object.
- Use setters to ensure only valid values can be set.
- Use getters and setters to create read only and write only values.
- Avoid the
this
token as it can be hijacked and means any code that uses the token can corrupt state.
Performance
After making state trustable you must address performance. I am often down voted for putting too much emphasis on performance, yet when a new app or framework comes out the reviews always make a point off performance. Poor performance is an app killer.
Performance starts at the very lowest level and is a mind set as much as a style. You should always be thinking, is there a faster way. You should also test and know what style are performant as in JavaScript it is not always evident.
Inline is faster than function calls. It is a balancing act between readability and performance, but when readability is marginal always go for the inline option.
You have
while(2*k <= this.n())
but could bewhile(2 * k < queue.length)
Note queue should not be exposed.
Don't repeat calculations
In swim you repeat the same calculation 3 times
Heap.flr(k/2)
. It should be calculated once and stored as a variable. You do the same in other functions.
Don't pass known values
You call
sink
andswim
with argumentk
, which are always the same value (either1
orqueue.length - 1
). There is no need to pass a known value.
Integers are faster than doubles
Javascript does not have a definable integer type, but internally to take advantage of hardware performance Javascript
Number
can be a variety of types from uint32 to double. All bitwise operators use uint32 and will coerce doubles to uint32. Integers can be up to 2 times faster than doubles (more depending on the platform)
You have
Heap.flr(k/2)
that will convert odd values ofk
to a double, then floor it inflr
which converts it back to integer. Also you require two allocations to the call stack just to complete the operation.k >> 2
does the same thing, avoids the intermediate double and requires no use of the call stack.
Learn binary math and how to use bitwise operators in javascript as they provide significant performance benefits when working with integers. Note that Javascript integers are signed 32 bit values.
Warning about destructuring
Though you have not used destructuring, it is nowadays common to perform a swap using destructing. eg
[queue[a], queue[b]] = [queue[b], queue[a]]
and as a syntax it is very nice, but it is also currently extremely slow (About 10 times slower). You can get the same performance doing{const swap = [queue[b], queue[a]]; const swapped = [swap[1], swap[0]]; queue[a] = swapped[0]; queue[b] = swap[1]}
to give a hint at how it's performed under the hood.
I am sure this problem will be addressed soon and performance will follow standard styles, but for now be wary of destructuring in terms of performance.
Consistency
Try to be consistent with JavaScript. If you access an array item outside the range you get undefined
. However in your code you return false
It makes more sense to return undefined
when the queue is empty.
The rewrite
The rewrite addresses some more performance issues. The swim function can perform a lot of swaps, while in reality you are only looking for a place to put a new value. Rather than add the new value to the queue, locate the new position and use splice to insert it. This save a lot of data movements. Thus swim
is passed the value to swim up and be inserted
sink
can also be improved by starting at 0, same with the queue
, there is no need to hold an unused item at the start.
Removing the first item from queue
means testing if there are items becomes if(queue.length)
If the queue
is empty then undefined is returned. This eliminates the need to add conditional code to test for empty.
As sink
is only called to shift a value from the queue, it may as well shift that value as well simplifying the delMax
function now called shift
The code is much safer this way as it can not be misused, and performs a lot faster. It also reduces the source code size by half thus making it much easier to understand.
function Heap() {
const queue = ;
function sinkAndShift() {
var k = 0, k2 = 1;
while (k2 < queue.length) {
if (queue[k] < queue[k2]) {
const tmp = queue[k]; // don't use destructuring.
queue[k] = queue[k2];
queue[k2] = tmp;
} else { break }
k2 = (k = k2) * 2;
}
return queue.shift();
}
function swim(val) {
var k = queue.length - 1;
while (k > 0 && val > queue[k]) { k >>= 1 }
queue.splice(k, 0, val);
}
return Object.freeze({
set value(k) {
if (isNaN(k)) { throw new RangeError("Must be a number '" + k + "'") }
swim(Number(k));
},
shift() { return sinkAndShift() },
get max() { return queue[0] },
get length() { return queue.length },
});
}
// usage
const heap = new Heap();
// or
//const heap = new Heap;
// or
//const heap = Heap();
heap.value = 100; // same as insert
heap.value = 101; // adds second value
heap.value = 102;
heap.value = 103;
console.log(heap.length); // outputs 4;
console.log(heap.max); // outputs 103;
console.log(heap.shift()); // outputs 103;
console.log(heap.length); // outputs 3;
console.log(heap.shift()); // outputs 102;
console.log(heap.shift()); // outputs 101;
console.log(heap.shift()); // outputs 100;
console.log(heap.shift()); // outputs undefined;
heap.value = "Monkey bread"; // will throw a range error
Thank you very much for your efforts
– Humoyun
Dec 17 at 19:38
add a comment |
Protect state
There is a fundamental overriding principle in programing that if correctly followed ensures that your code will work. Encapsulate and protect the objects state so that you can trust its integrity.
You have exposed the objects state yet provide no means to ensure that the state is maintained in such a way that your code will run as expected.
The following actions are all possible and all make the whole object unusable. You have no systematic way to determine if the state of heap is safe to use. Yet you continue to try and perform actions on a broken state, and will eventually throw an unknown error
const heap = new Heap();
heap.insert("ABC");
heap.queue.pop();
heap.queue.push(-100);
heap.queue.length = 0;
heap.queue = "monkey bread";
heap.exch(0,1000);
A good object means that there is NO WAY to corrupt the state of the object. That if the state cannot be maintained a predefined known list of errors will be thrown.
techniques to protect state
- Use closure to hide data that is not relevant to the user of the object.
- Use setters to ensure only valid values can be set.
- Use getters and setters to create read only and write only values.
- Avoid the
this
token as it can be hijacked and means any code that uses the token can corrupt state.
Performance
After making state trustable you must address performance. I am often down voted for putting too much emphasis on performance, yet when a new app or framework comes out the reviews always make a point off performance. Poor performance is an app killer.
Performance starts at the very lowest level and is a mind set as much as a style. You should always be thinking, is there a faster way. You should also test and know what style are performant as in JavaScript it is not always evident.
Inline is faster than function calls. It is a balancing act between readability and performance, but when readability is marginal always go for the inline option.
You have
while(2*k <= this.n())
but could bewhile(2 * k < queue.length)
Note queue should not be exposed.
Don't repeat calculations
In swim you repeat the same calculation 3 times
Heap.flr(k/2)
. It should be calculated once and stored as a variable. You do the same in other functions.
Don't pass known values
You call
sink
andswim
with argumentk
, which are always the same value (either1
orqueue.length - 1
). There is no need to pass a known value.
Integers are faster than doubles
Javascript does not have a definable integer type, but internally to take advantage of hardware performance Javascript
Number
can be a variety of types from uint32 to double. All bitwise operators use uint32 and will coerce doubles to uint32. Integers can be up to 2 times faster than doubles (more depending on the platform)
You have
Heap.flr(k/2)
that will convert odd values ofk
to a double, then floor it inflr
which converts it back to integer. Also you require two allocations to the call stack just to complete the operation.k >> 2
does the same thing, avoids the intermediate double and requires no use of the call stack.
Learn binary math and how to use bitwise operators in javascript as they provide significant performance benefits when working with integers. Note that Javascript integers are signed 32 bit values.
Warning about destructuring
Though you have not used destructuring, it is nowadays common to perform a swap using destructing. eg
[queue[a], queue[b]] = [queue[b], queue[a]]
and as a syntax it is very nice, but it is also currently extremely slow (About 10 times slower). You can get the same performance doing{const swap = [queue[b], queue[a]]; const swapped = [swap[1], swap[0]]; queue[a] = swapped[0]; queue[b] = swap[1]}
to give a hint at how it's performed under the hood.
I am sure this problem will be addressed soon and performance will follow standard styles, but for now be wary of destructuring in terms of performance.
Consistency
Try to be consistent with JavaScript. If you access an array item outside the range you get undefined
. However in your code you return false
It makes more sense to return undefined
when the queue is empty.
The rewrite
The rewrite addresses some more performance issues. The swim function can perform a lot of swaps, while in reality you are only looking for a place to put a new value. Rather than add the new value to the queue, locate the new position and use splice to insert it. This save a lot of data movements. Thus swim
is passed the value to swim up and be inserted
sink
can also be improved by starting at 0, same with the queue
, there is no need to hold an unused item at the start.
Removing the first item from queue
means testing if there are items becomes if(queue.length)
If the queue
is empty then undefined is returned. This eliminates the need to add conditional code to test for empty.
As sink
is only called to shift a value from the queue, it may as well shift that value as well simplifying the delMax
function now called shift
The code is much safer this way as it can not be misused, and performs a lot faster. It also reduces the source code size by half thus making it much easier to understand.
function Heap() {
const queue = ;
function sinkAndShift() {
var k = 0, k2 = 1;
while (k2 < queue.length) {
if (queue[k] < queue[k2]) {
const tmp = queue[k]; // don't use destructuring.
queue[k] = queue[k2];
queue[k2] = tmp;
} else { break }
k2 = (k = k2) * 2;
}
return queue.shift();
}
function swim(val) {
var k = queue.length - 1;
while (k > 0 && val > queue[k]) { k >>= 1 }
queue.splice(k, 0, val);
}
return Object.freeze({
set value(k) {
if (isNaN(k)) { throw new RangeError("Must be a number '" + k + "'") }
swim(Number(k));
},
shift() { return sinkAndShift() },
get max() { return queue[0] },
get length() { return queue.length },
});
}
// usage
const heap = new Heap();
// or
//const heap = new Heap;
// or
//const heap = Heap();
heap.value = 100; // same as insert
heap.value = 101; // adds second value
heap.value = 102;
heap.value = 103;
console.log(heap.length); // outputs 4;
console.log(heap.max); // outputs 103;
console.log(heap.shift()); // outputs 103;
console.log(heap.length); // outputs 3;
console.log(heap.shift()); // outputs 102;
console.log(heap.shift()); // outputs 101;
console.log(heap.shift()); // outputs 100;
console.log(heap.shift()); // outputs undefined;
heap.value = "Monkey bread"; // will throw a range error
Protect state
There is a fundamental overriding principle in programing that if correctly followed ensures that your code will work. Encapsulate and protect the objects state so that you can trust its integrity.
You have exposed the objects state yet provide no means to ensure that the state is maintained in such a way that your code will run as expected.
The following actions are all possible and all make the whole object unusable. You have no systematic way to determine if the state of heap is safe to use. Yet you continue to try and perform actions on a broken state, and will eventually throw an unknown error
const heap = new Heap();
heap.insert("ABC");
heap.queue.pop();
heap.queue.push(-100);
heap.queue.length = 0;
heap.queue = "monkey bread";
heap.exch(0,1000);
A good object means that there is NO WAY to corrupt the state of the object. That if the state cannot be maintained a predefined known list of errors will be thrown.
techniques to protect state
- Use closure to hide data that is not relevant to the user of the object.
- Use setters to ensure only valid values can be set.
- Use getters and setters to create read only and write only values.
- Avoid the
this
token as it can be hijacked and means any code that uses the token can corrupt state.
Performance
After making state trustable you must address performance. I am often down voted for putting too much emphasis on performance, yet when a new app or framework comes out the reviews always make a point off performance. Poor performance is an app killer.
Performance starts at the very lowest level and is a mind set as much as a style. You should always be thinking, is there a faster way. You should also test and know what style are performant as in JavaScript it is not always evident.
Inline is faster than function calls. It is a balancing act between readability and performance, but when readability is marginal always go for the inline option.
You have
while(2*k <= this.n())
but could bewhile(2 * k < queue.length)
Note queue should not be exposed.
Don't repeat calculations
In swim you repeat the same calculation 3 times
Heap.flr(k/2)
. It should be calculated once and stored as a variable. You do the same in other functions.
Don't pass known values
You call
sink
andswim
with argumentk
, which are always the same value (either1
orqueue.length - 1
). There is no need to pass a known value.
Integers are faster than doubles
Javascript does not have a definable integer type, but internally to take advantage of hardware performance Javascript
Number
can be a variety of types from uint32 to double. All bitwise operators use uint32 and will coerce doubles to uint32. Integers can be up to 2 times faster than doubles (more depending on the platform)
You have
Heap.flr(k/2)
that will convert odd values ofk
to a double, then floor it inflr
which converts it back to integer. Also you require two allocations to the call stack just to complete the operation.k >> 2
does the same thing, avoids the intermediate double and requires no use of the call stack.
Learn binary math and how to use bitwise operators in javascript as they provide significant performance benefits when working with integers. Note that Javascript integers are signed 32 bit values.
Warning about destructuring
Though you have not used destructuring, it is nowadays common to perform a swap using destructing. eg
[queue[a], queue[b]] = [queue[b], queue[a]]
and as a syntax it is very nice, but it is also currently extremely slow (About 10 times slower). You can get the same performance doing{const swap = [queue[b], queue[a]]; const swapped = [swap[1], swap[0]]; queue[a] = swapped[0]; queue[b] = swap[1]}
to give a hint at how it's performed under the hood.
I am sure this problem will be addressed soon and performance will follow standard styles, but for now be wary of destructuring in terms of performance.
Consistency
Try to be consistent with JavaScript. If you access an array item outside the range you get undefined
. However in your code you return false
It makes more sense to return undefined
when the queue is empty.
The rewrite
The rewrite addresses some more performance issues. The swim function can perform a lot of swaps, while in reality you are only looking for a place to put a new value. Rather than add the new value to the queue, locate the new position and use splice to insert it. This save a lot of data movements. Thus swim
is passed the value to swim up and be inserted
sink
can also be improved by starting at 0, same with the queue
, there is no need to hold an unused item at the start.
Removing the first item from queue
means testing if there are items becomes if(queue.length)
If the queue
is empty then undefined is returned. This eliminates the need to add conditional code to test for empty.
As sink
is only called to shift a value from the queue, it may as well shift that value as well simplifying the delMax
function now called shift
The code is much safer this way as it can not be misused, and performs a lot faster. It also reduces the source code size by half thus making it much easier to understand.
function Heap() {
const queue = ;
function sinkAndShift() {
var k = 0, k2 = 1;
while (k2 < queue.length) {
if (queue[k] < queue[k2]) {
const tmp = queue[k]; // don't use destructuring.
queue[k] = queue[k2];
queue[k2] = tmp;
} else { break }
k2 = (k = k2) * 2;
}
return queue.shift();
}
function swim(val) {
var k = queue.length - 1;
while (k > 0 && val > queue[k]) { k >>= 1 }
queue.splice(k, 0, val);
}
return Object.freeze({
set value(k) {
if (isNaN(k)) { throw new RangeError("Must be a number '" + k + "'") }
swim(Number(k));
},
shift() { return sinkAndShift() },
get max() { return queue[0] },
get length() { return queue.length },
});
}
// usage
const heap = new Heap();
// or
//const heap = new Heap;
// or
//const heap = Heap();
heap.value = 100; // same as insert
heap.value = 101; // adds second value
heap.value = 102;
heap.value = 103;
console.log(heap.length); // outputs 4;
console.log(heap.max); // outputs 103;
console.log(heap.shift()); // outputs 103;
console.log(heap.length); // outputs 3;
console.log(heap.shift()); // outputs 102;
console.log(heap.shift()); // outputs 101;
console.log(heap.shift()); // outputs 100;
console.log(heap.shift()); // outputs undefined;
heap.value = "Monkey bread"; // will throw a range error
function Heap() {
const queue = ;
function sinkAndShift() {
var k = 0, k2 = 1;
while (k2 < queue.length) {
if (queue[k] < queue[k2]) {
const tmp = queue[k]; // don't use destructuring.
queue[k] = queue[k2];
queue[k2] = tmp;
} else { break }
k2 = (k = k2) * 2;
}
return queue.shift();
}
function swim(val) {
var k = queue.length - 1;
while (k > 0 && val > queue[k]) { k >>= 1 }
queue.splice(k, 0, val);
}
return Object.freeze({
set value(k) {
if (isNaN(k)) { throw new RangeError("Must be a number '" + k + "'") }
swim(Number(k));
},
shift() { return sinkAndShift() },
get max() { return queue[0] },
get length() { return queue.length },
});
}
// usage
const heap = new Heap();
// or
//const heap = new Heap;
// or
//const heap = Heap();
heap.value = 100; // same as insert
heap.value = 101; // adds second value
heap.value = 102;
heap.value = 103;
console.log(heap.length); // outputs 4;
console.log(heap.max); // outputs 103;
console.log(heap.shift()); // outputs 103;
console.log(heap.length); // outputs 3;
console.log(heap.shift()); // outputs 102;
console.log(heap.shift()); // outputs 101;
console.log(heap.shift()); // outputs 100;
console.log(heap.shift()); // outputs undefined;
heap.value = "Monkey bread"; // will throw a range error
function Heap() {
const queue = ;
function sinkAndShift() {
var k = 0, k2 = 1;
while (k2 < queue.length) {
if (queue[k] < queue[k2]) {
const tmp = queue[k]; // don't use destructuring.
queue[k] = queue[k2];
queue[k2] = tmp;
} else { break }
k2 = (k = k2) * 2;
}
return queue.shift();
}
function swim(val) {
var k = queue.length - 1;
while (k > 0 && val > queue[k]) { k >>= 1 }
queue.splice(k, 0, val);
}
return Object.freeze({
set value(k) {
if (isNaN(k)) { throw new RangeError("Must be a number '" + k + "'") }
swim(Number(k));
},
shift() { return sinkAndShift() },
get max() { return queue[0] },
get length() { return queue.length },
});
}
// usage
const heap = new Heap();
// or
//const heap = new Heap;
// or
//const heap = Heap();
heap.value = 100; // same as insert
heap.value = 101; // adds second value
heap.value = 102;
heap.value = 103;
console.log(heap.length); // outputs 4;
console.log(heap.max); // outputs 103;
console.log(heap.shift()); // outputs 103;
console.log(heap.length); // outputs 3;
console.log(heap.shift()); // outputs 102;
console.log(heap.shift()); // outputs 101;
console.log(heap.shift()); // outputs 100;
console.log(heap.shift()); // outputs undefined;
heap.value = "Monkey bread"; // will throw a range error
answered Dec 17 at 9:28
Blindman67
6,9771521
6,9771521
Thank you very much for your efforts
– Humoyun
Dec 17 at 19:38
add a comment |
Thank you very much for your efforts
– Humoyun
Dec 17 at 19:38
Thank you very much for your efforts
– Humoyun
Dec 17 at 19:38
Thank you very much for your efforts
– Humoyun
Dec 17 at 19:38
add a comment |
Thanks for contributing an answer to Code Review Stack Exchange!
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
Use MathJax to format equations. MathJax reference.
To learn more, see our tips on writing great answers.
Some of your past answers have not been well-received, and you're in danger of being blocked from answering.
Please pay close attention to the following guidance:
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
To learn more, see our tips on writing great answers.
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f209769%2fmaxheap-implementation-in-javascript%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown