Stack Implementation in JavaScript

Question

I am learning data structures and below is my implementation of a stack in JavaScript. I didn't want to use built in functions for an array because in JavaScript that would be too easy, so I made my own.

class Stack {
 constructor(data=null) {
 this.items = [data]
 this.length = 0
 }
 push(data) {
 this.items[this.length] = data
 this.length++
 }
 pop() {
 delete this.items[this.length-1]
 this.length--
 }
 printStack() {
 for(let i = 0; i<this.length; i++)
 console.log(this.items[i])
 }
 isEmpty() {
 return (this.length) > 0 ? false : true
 }
}
let myStack = new Stack();
myStack.push(123)
myStack.push(456)
myStack.pop()
myStack.printStack()
console.log(myStack.isEmpty())

Answer 1

You have a bug in your constructor:

constructor(data=null) {
 this.items = [data]
 this.length = 0
 }

If you pass an item and then push to the stack, you lose the first item:

let myStack = new Stack("123");
myStack.push("456")
// myStack.items = ["456"]

You need to make sure that you initialize length to 1 if an item is passed in. The simplest way to do that is something like this:

constructor(datum = null) {
 if (datum !== null) {
 this.items = [datum];
 this.length = 1;
 } else {
 this.items = [];
 this.length = 0;
 }
}

Some people like to prefix private fields with _ to show that they aren't supposed to be used externally. You could consider using private names but they aren't supported everywhere.

Your pop function should return the element that was popped from the stack to be useful.

You can also simplify isEmpty:

isEmpty() {
 return this.length === 0;
}

Answer 2

Encapsulate

The object Stack should protect its state as it is easy to add and remove items via the exposed referenced Stack.items, or if Stack.length were to be set some erroneous value eg myStack.length = 1.001

You can never guarantee your object will act like a stack if you can not trust its state.

Review

Most points are already covered by accepted answer, however there are some additional points.

• Avoid using null

• Stack.pop does not return anything? What is the point of a stack that does not provide access to the top item.

• Use getters and setters rather than give direct access to state critical properties.

• Rather than Stack.printStack, use a getter to get a copy of the stack that you can then use to log to the console or anything else.

• Spaces between operators. eg [this.length - 1] not [this.length-1]

• Always delimit code blocks. for( ; ; ) /* code */ should be for( ; ; ) { /* code */ }

• Avoid redundant or long form code.

  You had delete this.items[this.length-1]; this.length-- could have been a single line delete this.items[--this.length]

Rewrite

The rewrite creates a stack using a factory function. The Stack is frozen to ensure it's state can be trusted.

function Stack(...items) {
 const stack = items;
 return Object.freeze({
 push(...items) { stack.push(...items) },
 pop() { return stack.pop() },
 clear() { stack.length = 0 },
 get size() { return stack.length },
 get isEmpty() { return !stack.length },
 get items() { return [...stack] },
 }); 
}
// usage
const myStack = Stack(1, 2, 3);
myStack.push(123, 436, 512);
console.log(myStack.items.join(","));
while (!myStack.isEmpty) { 
 console.log("Pop item[" + (myStack.size - 1) + "]: " + myStack.pop());
}

• Jordan Baron: THank you! And just to check my knowledge, the returned object has access to the stack variable because it forms a closure since Object.freeze is a function?
• TheRubberDuck: @JordanBaron It doesn't matter that Object.freeze is a function, but that the frozen object contains functions. Each of push, pop, clear, size, isEmpty, and items is a separate closure that has has access to this specific copy of stack after being returned.
• Blindman67: @JordanBaron Exactly what @TheRubberDuck commented. However to clarify terminology. As used in the rewrite Object.freeze is a "function call", while the object it freezes contains "function declarations" push, pop, ... . Only function declarations / expressions can close over (closure) variables within scope
• Voo: "Avoid redundant or long form code.". Using -- or ++ as part of a larger expression is such an incredibly common cause of errors and confusion that languages worrying about clarity explicitly forbid this.
• Blindman67: @Voo At CodeReview I endeavor to review code at professional level. Wide spread miss understanding of the pre/post decrement/increment operator is not a reason to avoid its use, rather it is a reason to improve education and training. The mind set of "too hard don't use" helps no one and does nothing but hold the industry back.