Suppressing warnings

It's hard to defend suppressing warnings at the class level.The problematic lines in your code are these:

    data = (T[]) new Object[INITIAL_CAPACITY];

Which appears in two places.I suggest to move the array allocation to one place,and suppress the warning only for that line, like this:

public ArrayStack() {    data = newArray(INITIAL_CAPACITY);}private final T[] newArray(int capacity) {    @SuppressWarnings("unchecked")    T[] tmp = (T[]) new Object[capacity];    return tmp;}private void enlarge() {    T[] temp = Arrays.copyOf(data, data.length);    data = newArray(data.length * 2);    System.arraycopy(temp, 0, data, 0, temp.length);}

This hits two birds with one stone:

• The ugly array creation is now at once place instead of two
• Warnings are suppressed for the single problematic line, not for the entire class

In case you were wondering, the localtmp variable is necessary innewArray, otherwise Java doesn't allow suppressing the warning on a return statement or regular assignment. It has to be declaration + assignment.

Another important note here is theprivate final modifier ofnewArray.Theprivate modifier is necessary to prevent sub-classes from overriding the behavior and potentially breaking the constructor.Thefinal modifier is necessary to prevent sub-classes from shadowing the method by another method with the same name, causing confusion.

Manual array copy

Instead of this:

for (int i = 0; i < temp.length; i++) {    data[i] = temp[i];}

It's better to useSystem.arraycopy instead:

System.arraycopy(temp, 0, data, 0, temp.length);

(Also seen in the previous section.)

Other minor things

Inclear, I'm wondering why you manually null out the elements,rather than simply creating a fresh new array.You could let the garbage collector take care of the stale array.

Random is unused, so remove the import.

• \$\begingroup\$I wasn't sure what is better, and I thought that creating a new array was not necessary\$\endgroup\$

  Stefan Rendevski

  – Stefan Rendevski

  2015-11-26 19:09:24 +00:00

  CommentedNov 26, 2015 at 19:09
• \$\begingroup\$I don't know which is uglier...(T[])new Object[], or the alternative, which is using anObject array and casting to T every time you get something out of the array\$\endgroup\$

  Stack Exchange Broke The Law

  – Stack Exchange Broke The Law

  2015-11-27 05:10:11 +00:00

  CommentedNov 27, 2015 at 5:10
• 2
  \$\begingroup\$Isolating the ugliness to one place, protecting the rest of the code from it is better\$\endgroup\$

  janos

  – janos

  2015-11-27 06:05:23 +00:00

  CommentedNov 27, 2015 at 6:05

Manual clearing array

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

Single line if statements

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

This has 2 code smells:

• Usesif without braces. Semantically correct, but error prone during subsequent modifications. A lot of style guides plainly disallow this
• Has code in the same line asif 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 havestatic 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 explicittrim and you loose it, although in a controlled manner. Add an implicittrim (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).

Unnecessary Copy

Currently you make a copy of the original array before you enlarge it:

T[] temp = Arrays.copyOf(data, data.length);

But you don't need to make a copy of that array. You could keep a reference to it like this:

 T[] temp = data;

The rest of the code will work the same using the reference to the old array instead of a copy of it.

• \$\begingroup\$Wow, nice, wouldn't have remembered that\$\endgroup\$

  Stefan Rendevski

  – Stefan Rendevski

  2015-11-26 21:29:43 +00:00

  CommentedNov 26, 2015 at 21:29

Simple enlarge

The enlarge function can be simplified to

private void enlarge() {    data = Arrays.copyOf(data, data.length * 2);}

because the Arrays.copyOf function will automatically truncate or pad given an array size different to the original.

This allows you to completely avoid some of the issues that other users have mentioned, like the unnecessary temp copy and the warning on the cast.

• \$\begingroup\$Neat, I didn't knowArrays.copyOf() can pad.\$\endgroup\$

  Stefan Rendevski

  – Stefan Rendevski

  2015-11-27 00:23:47 +00:00

  CommentedNov 27, 2015 at 0:23
• \$\begingroup\$@StefanRendevski checking documentation even for simple functions is a good idea, as well designed APIs often have sensible/useful behaviour for common edge cases.\$\endgroup\$

  Edward Peek

  – Edward Peek

  2015-11-27 00:29:39 +00:00

  CommentedNov 27, 2015 at 0:29

• java
• stack