Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

##2) Use this to attach attributes to a Constructor. More info More info

##2) Use this to attach attributes to a Constructor. More info

##2) Use this to attach attributes to a Constructor. More info

Source Link
Larry Battle
  • 2.2k
  • 11
  • 19

You're off to a great start. +1 for providing comments.

Here are some tips:

##1) Define variables before modifying them. So the constants MAX_HEAP and MIN_HEAP should be defined after PriorityQueue.

Old Code:

PriorityQueue.MAX_HEAP = 0;
PriorityQueue.MIN_HEAP = 1;
function PriorityQueue(criteria, heapType) {
 //...
}

New Code:

function PriorityQueue(criteria, heapType) {
 //...
}
PriorityQueue.MAX_HEAP = 0;
PriorityQueue.MIN_HEAP = 1;

Side note: In shiftHighestPriorityElement(), _queue isn't defined. I think it should be queue.

##2) Use this to attach attributes to a Constructor. More info

Example: Old Code:

//..
function PriorityQueue(criteria, heapType) {
 this.length = 0;
 var queue = [];
 var isMax = false;
 //...

New Code:

//..
function PriorityQueue(criteria, heapType) {
 this.length = 0;
 this.queue = [];
 this.isMax = false;
//...

##3) Split up functions longer than 8 - 12 lines of code into smaller functions. Use the prototype on an object to attach functions to the constructor instead of including all the functionality within.

Old Code:

function PriorityQueue(criteria, heapType) {
 //...
 this.insert = function (value) {
 if (!value.hasOwnProperty(criteria)) {
 throw "Cannot insert " + value + " because it does not have a property by the name of " + criteria + ".";
 }
 queue.push(value);
 length++;
 bubbleUp(length - 1);
 }
 //...
 var swap = function (self, target) {
 var placeHolder = queue[self];
 queue[self] = queue[target];
 queue[target] = placeHolder;
 }
 //...
}

New Code:

function PriorityQueue(criteria, heapType) {
//...
}
PriorityQueue.prototype.insert = function (value) {
 if (!value.hasOwnProperty(this.criteria)) {
 throw "Cannot insert " + value + " because it does not have a property by the name of " + this.criteria + ".";
 }
 this.queue.push(value);
 this.length++;
 this.bubbleUp(this.length - 1);
}
PriorityQueue.prototype.swap = function (self, target) {
 var placeHolder = this.queue[self];
 this.queue[self] = this.queue[target];
 this.queue[target] = placeHolder;
}

##4) Sometimes it's better to just return an boolean expression.

Old Code:

if (queue[self][criteria] < queue[target][criteria]) {
 return true;
} else {
 return false;
}

New Code:

return (queue[self][criteria] < queue[target][criteria]);

##5) Make sure to create test units. Try out qunit.js

##6) Simplify if and else conditions.

Old Code:

var isMax = false;
//Constructor
if (heapType==PriorityQueue.MAX_HEAP) {
 isMax = true;
} else if (heapType==PriorityQueue.MIN_HEAP) {
 isMax = false;
} else {
 throw heapType + " not supported.";
}

New Code:

this.isMax = !!heapType;
if ( heapType !== 0 || heapType !== 1 ){
 throw heapType + " not supported.";
}

##7) Use === instead of == when comparing for value and type of primitive values.

Old code:

} else if (value == 0) {

New Code:

} else if (value === 0) {

Or

} else if (!value) { 

##Final code:

function PriorityQueue(criteria, heapType) {
 this.criteria = criteria;
 this.length = 0;
 this.queue = [];
 this.isMax = !!heapType;
 if ( heapType !== 0 || heapType !== 1 ){
 throw heapType + " not supported.";
 }
}
PriorityQueue.prototype.insert = function (value) {
 if (!value.hasOwnProperty(this.criteria)) {
 throw "Cannot insert " + value + " because it does not have a property by the name of " + this.criteria + ".";
 }
 this.queue.push(value);
 this.length++;
 this.bubbleUp(this.length - 1);
};
PriorityQueue.prototype.getHighestPriorityElement = function () {
 return this.queue[0];
};
PriorityQueue.prototype.shiftHighestPriorityElement = function () {
 if (length < 0) {
 throw("There are no more elements in your priority queue.");
 }
 var oldRoot = this.queue[0];
 var newRoot = this.queue.pop();
 this.length--;
 this.queue[0] = newRoot;
 this.swapUntilQueueIsCorrect(0);
 return oldRoot;
};
PriorityQueue.prototype.bubbleUp = function (index) {
 if (index === 0) {
 return;
 }
 var parent = this.getParentOf(index);
 if (this.evaluate(index, parent)) {
 this.swap(index, parent);
 this.bubbleUp(parent);
 } else {
 return;
 }
};
PriorityQueue.prototype.swapUntilQueueIsCorrect = function (value) {
 var left = this.getLeftOf(value),
 right = this.getRightOf(value);
 
 if (this.evaluate(left, value)) {
 this.swap(value, left);
 this.swapUntilQueueIsCorrect(left);
 } else if (this.evaluate(right, value)) {
 this.swap(value, right);
 this.swapUntilQueueIsCorrect(right);
 } else if (value === 0) {
 return;
 } else {
 this.swapUntilQueueIsCorrect(0);
 }
};
PriorityQueue.prototype.swap = function (self, target) {
 var placeHolder = this.queue[self];
 this.queue[self] = this.queue[target];
 this.queue[target] = placeHolder;
};
PriorityQueue.prototype.evaluate = function (self, target) {
 if (this.queue[target] === null || this.queue[self] === null) {
 return false;
 }
 if (this.isMax) {
 return (this.queue[self][this.criteria] > this.queue[target][this.criteria]);
 } else {
 return (this.queue[self][this.criteria] < this.queue[target][this.criteria]);
 }
};
PriorityQueue.prototype.getParentOf = function (index) {
 return Math.floor(index / 2) - 1;
};
PriorityQueue.prototype.getLeftOf = function (index) {
 return index * 2 + 1;
};
PriorityQueue.prototype.getRightOf = function (index) {
 return index * 2 + 2;
};
PriorityQueue.MAX_HEAP = 0;
PriorityQueue.MIN_HEAP = 1;
default

AltStyle によって変換されたページ (->オリジナル) /