Return to Answer

@PhilWright made some good points. I would add:

• Popping element from an empty stack should throw java.util.EmptyStackException
• Test for memory leaks. Since the Stack is in charge of managing its own memory, this is a real concern. A common memory leak in stack implementations is when removed elements are not nulled out correctly.

Here's one way to verify that pop cleans up after the removed object, based on @DavidHarkness' idea in comments, using a WeakReference:

@Test
public void testPopCleansUpReference() {
    Object value = new Object();
    WeakReference<Object> reference = new WeakReference<>(value);

    Stack<Object> stack = new StackImpl();
    stack.push(value);
    value = null;

    stack.pop();
    Runtime.getRuntime().gc();
    assertNull(reference.get());
}

The current implementation fails this test, because this implementation only decreases the size variable, the popped object remains inside the data array:

@Override
public Object pop() {
    --size;
    return data[size];
}

For the record, the fix is simple enough:

@Override
public Object pop() {
    Object result = data[--size];
    data[size] = null;
    return result;
}

@PhilWright made some good points. I would add:

• Popping element from an empty stack should throw java.util.EmptyStackException
• Test for memory leaks. Since the Stack is in charge of managing its own memory, this is a real concern. A common memory leak in stack implementations is when removed elements are not nulled out correctly.

Here's one way to verify that pop cleans up after the removed object, based on @DavidHarkness' idea in comments, using a WeakReference:

@Test
public void testPopCleansUpReference() {
    Object value = new Object();
    WeakReference<Object> reference = new WeakReference<>(value);

    Stack<Object> stack = new StackImpl();
    stack.push(value);
    value = null;

    stack.pop();
    Runtime.getRuntime().gc();
    assertNull(reference.get());
}

The current implementation fails this test, because this implementation only decreases the size variable, the popped object remains inside the data array:

@Override
public Object pop() {
    --size;
    return data[size];
}

For the record, the fix is simple enough:

@Override
public Object pop() {
    Object result = data[--size];
    data[size] = null;
    return result;
}

@PhilWright made some good points. I would add:

• Popping element from an empty stack should throw EmptyStackException
• Test for memory leaks. Since the Stack is in charge of managing its own memory, this is a real concern. A common memory leak in stack implementations is when removed elements are not nulled out correctly.

Here's one way to verify that pop cleans up after the removed object, based on @DavidHarkness' idea in comments, using a WeakReference:

@Test
public void testPopCleansUpReference() {
    Object value = new Object();
    WeakReference<Object> reference = new WeakReference<>(value);

    Stack<Object> stack = new StackImpl();
    stack.push(value);
    value = null;

    stack.pop();
    Runtime.getRuntime().gc();
    assertNull(reference.get());
}

The current implementation fails this test, because this implementation only decreases the size variable, the popped object remains inside the data array:

@Override
public Object pop() {
    --size;
    return data[size];
}

For the record, the fix is simple enough:

@Override
public Object pop() {
    Object result = data[--size];
    data[size] = null;
    return result;
}

@PhilWright made some good points. I would add:

• Popping element from an empty stack should throw java.util.EmptyStackException
• Test for memory leaks. Since the Stack is in charge of managing its own memory, this is a real concern. A common memory leak in stack implementations is when removed elements are not nulled out correctly.

Here's one way to verify that pop cleans up after the removed object, based on @DavidHarkness' idea in comments, using a WeakReference:

@Test
public void testPopCleansUpReference() {
    Object value = new Object();
    WeakReference<Object> reference = new WeakReference<>(value);

    Stack<Object> stack = new StackImpl();
    stack.push(value);
    value = null;

    stack.pop();
    Runtime.getRuntime().gc();
    assertNull(reference.get());
}

The current implementation fails this test, because this implementation only decreases the size variable, the popped object remains inside the data array:

@Override
public Object pop() {
    --size;
    return data[size];
}

For the record, the fix is simple enough:

@Override
public Object pop() {
    Object result = data[--size];
    data[size] = null;
    return result;
}

@PhilWright made some good points. I would add:

• Popping element from an empty stack should throw java.util.EmptyStackException
• Test for memory leaks. Since the Stack is in charge of managing its own memory, this is a real concern. A common memory leak in stack implementations is when removed elements are not nulled out correctly. To test this:
  • Check memory use for a baseline
  • Add many objects to the stack
  • Check memory use again
  • Remove all items from the stack
  • Check memory use again. If there is no memory leak, the memory use should be closer to the baseline than the previous checkpoint

Here's one way to check the amount of available memory, but there might be better:

long getAvailableMemory() {
    Runtime runtime = Runtime.getRuntime();
    runtime.gc();
    return runtime.maxMemory() - runtime.totalMemory() + runtime.freeMemory();
}

You wrote that you're not interested in comments on the implementation itself, perhaps the memory link in it is worth mentioning:

@Override
public Object pop() {
    --size;
    return data[size];
}

Only size is decreased, the reference to the item that was supposed to be removed is still there, and so it won't be garbage collected. Here's the fix:

@Override
public Object pop() {
    Object result = data[--size];
    data[size] = null;
    return result;
}

@PhilWright made some good points. I would add:

• Popping element from an empty stack should throw EmptyStackException
• Test for memory leaks. Since the Stack is in charge of managing its own memory, this is a real concern. A common memory leak in stack implementations is when removed elements are not nulled out correctly.

Here's one way to verify that pop cleans up after the removed object, based on @DavidHarkness' idea in comments, using a WeakReference:

@Test
public void testPopCleansUpReference() {
    Object value = new Object();
    WeakReference<Object> reference = new WeakReference<>(value);

    Stack<Object> stack = new StackImpl();
    stack.push(value);
    value = null;

    stack.pop();
    Runtime.getRuntime().gc();
    assertNull(reference.get());
}

The current implementation fails this test, because this implementation only decreases the size variable, the popped object remains inside the data array:

@Override
public Object pop() {
    --size;
    return data[size];
}

For the record, the fix is simple enough:

@Override
public Object pop() {
    Object result = data[--size];
    data[size] = null;
    return result;
}