MaxHeap implementation in JavaScript












4














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?










share|improve this question



























    4














    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?










    share|improve this question

























      4












      4








      4







      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?










      share|improve this question













      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






      share|improve this question













      share|improve this question











      share|improve this question




      share|improve this question










      asked Dec 16 at 16:00









      Humoyun

      1405




      1405






















          2 Answers
          2






          active

          oldest

          votes


















          4














          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 with flr(k / 2) you could name it childIndex and call it with k, letting it do the necessary math


          • flr uses parameter i. The name i is best in simple counting loops.






          share|improve this answer



















          • 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














          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 be while(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 and swim with argument k, which are always the same value (either 1 or queue.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 of k to a double, then floor it in flr 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








          share|improve this answer





















          • Thank you very much for your efforts
            – Humoyun
            Dec 17 at 19:38











          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
          });


          }
          });














          draft saved

          draft discarded


















          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









          4














          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 with flr(k / 2) you could name it childIndex and call it with k, letting it do the necessary math


          • flr uses parameter i. The name i is best in simple counting loops.






          share|improve this answer



















          • 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
















          4














          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 with flr(k / 2) you could name it childIndex and call it with k, letting it do the necessary math


          • flr uses parameter i. The name i is best in simple counting loops.






          share|improve this answer



















          • 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














          4












          4








          4






          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 with flr(k / 2) you could name it childIndex and call it with k, letting it do the necessary math


          • flr uses parameter i. The name i is best in simple counting loops.






          share|improve this answer














          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 with flr(k / 2) you could name it childIndex and call it with k, letting it do the necessary math


          • flr uses parameter i. The name i is best in simple counting loops.







          share|improve this answer














          share|improve this answer



          share|improve this answer








          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














          • 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













          1














          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 be while(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 and swim with argument k, which are always the same value (either 1 or queue.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 of k to a double, then floor it in flr 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








          share|improve this answer





















          • Thank you very much for your efforts
            – Humoyun
            Dec 17 at 19:38
















          1














          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 be while(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 and swim with argument k, which are always the same value (either 1 or queue.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 of k to a double, then floor it in flr 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








          share|improve this answer





















          • Thank you very much for your efforts
            – Humoyun
            Dec 17 at 19:38














          1












          1








          1






          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 be while(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 and swim with argument k, which are always the same value (either 1 or queue.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 of k to a double, then floor it in flr 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








          share|improve this answer












          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 be while(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 and swim with argument k, which are always the same value (either 1 or queue.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 of k to a double, then floor it in flr 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






          share|improve this answer












          share|improve this answer



          share|improve this answer










          answered Dec 17 at 9:28









          Blindman67

          6,9771521




          6,9771521












          • 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




          Thank you very much for your efforts
          – Humoyun
          Dec 17 at 19:38


















          draft saved

          draft discarded




















































          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.




          draft saved


          draft discarded














          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





















































          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







          Popular posts from this blog

          Сан-Квентин

          8-я гвардейская общевойсковая армия

          Алькесар