Skip to main content
added 721 characters in body
Source Link
Torinthiel
  • 221
  • 1
  • 3

Manual clearing array

In clear you manually null out the elements. You could re-initialize the array (which will throw work at the GC) or you could use Arrays.fill(data, null) or use the fact that larger indexes are already null and use Arrays.fill(data, 0, numElements-1, null) with adding a check for empty stack, all this with java.util.Arrays.

Single line if statements

if (isEmpty()) throw new EmptyStackException();

This has 2 code smells:

  • Uses if without braces. Semantically correct, but error prone during subsequent modifications. A lot of style guides plainly disallow this
  • Has code in the same line as if statement. Sometimes seen as less readable.

Seeing as what it does, I'd have

private ensureNonEmpty() {
    if (isEmpty()) {
        throw new EmptyStackException();
    }
}

and use it in both places. Would be also easier to change (and harder to skip one) if you later want to swap this exception for a different one.

Order of declarations

Convention is to have static members before non-static ones. So it's

private static final int INITIAL_CAPACITY = 10;
private T[] data;
private int numElements;

Or even better

private static final int INITIAL_CAPACITY = 10;

private T[] data;
private int numElements;

With some separation between static and non-static.

Shrinking the array

Should I reduce the capacity of the stack at some point? Testing the native Java Stack, it doesn't seem to do any size reduction, except with trim().

Currently your code has one important characteristics: Amortized constant time push cost. While a single push can be not constant time, the average of all push operations (no matter how many pop/clear you do between) stays constant. Add an explicit trim and you loose it, although in a controlled manner. Add an implicit trim (say when the array is one-third full) and you loose this completely. I'd rather not do it or explicitly warn about it in the JavaDoc (And I'm not complaining about lack of one, as you've admitted yourself).

Manual clearing array

In clear you manually null out the elements. You could re-initialize the array (which will throw work at the GC) or you could use Arrays.fill(data, null) or use the fact that larger indexes are already null and use Arrays.fill(data, 0, numElements-1, null) with adding a check for empty stack, all this with java.util.Arrays.

Single line if statements

if (isEmpty()) throw new EmptyStackException();

This has 2 code smells:

  • Uses if without braces. Semantically correct, but error prone during subsequent modifications. A lot of style guides plainly disallow this
  • Has code in the same line as if statement. Sometimes seen as less readable.

Seeing as what it does, I'd have

private ensureNonEmpty() {
    if (isEmpty()) {
        throw new EmptyStackException();
    }
}

and use it in both places. Would be also easier to change (and harder to skip one) if you later want to swap this exception for a different one.

Order of declarations

Convention is to have static members before non-static ones. So it's

private static final int INITIAL_CAPACITY = 10;
private T[] data;
private int numElements;

Or even better

private static final int INITIAL_CAPACITY = 10;

private T[] data;
private int numElements;

With some separation between static and non-static.

Manual clearing array

In clear you manually null out the elements. You could re-initialize the array (which will throw work at the GC) or you could use Arrays.fill(data, null) or use the fact that larger indexes are already null and use Arrays.fill(data, 0, numElements-1, null) with adding a check for empty stack, all this with java.util.Arrays.

Single line if statements

if (isEmpty()) throw new EmptyStackException();

This has 2 code smells:

  • Uses if without braces. Semantically correct, but error prone during subsequent modifications. A lot of style guides plainly disallow this
  • Has code in the same line as if statement. Sometimes seen as less readable.

Seeing as what it does, I'd have

private ensureNonEmpty() {
    if (isEmpty()) {
        throw new EmptyStackException();
    }
}

and use it in both places. Would be also easier to change (and harder to skip one) if you later want to swap this exception for a different one.

Order of declarations

Convention is to have static members before non-static ones. So it's

private static final int INITIAL_CAPACITY = 10;
private T[] data;
private int numElements;

Or even better

private static final int INITIAL_CAPACITY = 10;

private T[] data;
private int numElements;

With some separation between static and non-static.

Shrinking the array

Should I reduce the capacity of the stack at some point? Testing the native Java Stack, it doesn't seem to do any size reduction, except with trim().

Currently your code has one important characteristics: Amortized constant time push cost. While a single push can be not constant time, the average of all push operations (no matter how many pop/clear you do between) stays constant. Add an explicit trim and you loose it, although in a controlled manner. Add an implicit trim (say when the array is one-third full) and you loose this completely. I'd rather not do it or explicitly warn about it in the JavaDoc (And I'm not complaining about lack of one, as you've admitted yourself).

Source Link
Torinthiel
  • 221
  • 1
  • 3

Manual clearing array

In clear you manually null out the elements. You could re-initialize the array (which will throw work at the GC) or you could use Arrays.fill(data, null) or use the fact that larger indexes are already null and use Arrays.fill(data, 0, numElements-1, null) with adding a check for empty stack, all this with java.util.Arrays.

Single line if statements

if (isEmpty()) throw new EmptyStackException();

This has 2 code smells:

  • Uses if without braces. Semantically correct, but error prone during subsequent modifications. A lot of style guides plainly disallow this
  • Has code in the same line as if statement. Sometimes seen as less readable.

Seeing as what it does, I'd have

private ensureNonEmpty() {
    if (isEmpty()) {
        throw new EmptyStackException();
    }
}

and use it in both places. Would be also easier to change (and harder to skip one) if you later want to swap this exception for a different one.

Order of declarations

Convention is to have static members before non-static ones. So it's

private static final int INITIAL_CAPACITY = 10;
private T[] data;
private int numElements;

Or even better

private static final int INITIAL_CAPACITY = 10;

private T[] data;
private int numElements;

With some separation between static and non-static.