5
\$\begingroup\$

I have created a simple ArrayList with some methods from the actual ArrayList in Java.

I am testing this for mistakes, but I am learning Java and my tests are maybe not comprehensive.

public class SJUArrayList<E> {
    // Data fields
    private E[] theData;
    private int size = 0;
    private int capacity = 0;

    // Constants
    private static final int INIT_CAPACITY = 10;

    // Constructors
    public SJUArrayList() {
        this(INIT_CAPACITY);
    }

    public SJUArrayList(int initCapacity) {
        capacity = initCapacity;
        theData = (E[]) new Object[capacity];
    }

    // Methods
    public boolean add(E e) {
        if(e == null) {
            throw new NullPointerException();
        }

        if(size == capacity) {
            reallocate();
        }

        theData[size] = e;
        size++;

        return true;
    } // End add(E e) method

    public void add(int index, E e) {
        if(index < 0 || index > size) {
            throw new ArrayIndexOutOfBoundsException(index);
        }

        if(e == null) {
            throw new NullPointerException();
        }

        if(size == capacity) {
            reallocate();
        }

        for(int i = size; i > index; i--) {
            theData[i] = theData[i - 1];
        }

        theData[index] = e;
        size++;
    } // End add(int index, E e) method

    public void clear() {
        theData = (E[]) new Object[capacity];
        size = 0;
    } // End clear() method

    public boolean equals(Object o) {
        if(o == null) {
            return false;
        }

        if(getClass() != o.getClass()) {
            return false;
        }

        SJUArrayList<E> otherO = (SJUArrayList<E>) o;

        if(size != otherO.size) {
            return false;
        }

        for(int i = 0; i < size; i++) {
            if(!theData[i].equals(otherO.theData[i])) {
                return false;
            }
        }

        return true;
    } // End equals(Object o) method

    public E get(int index) {
        if(index < 0 || index >= size) {
            throw new ArrayIndexOutOfBoundsException(index);
        }
        return theData[index];
    } // End get(int index) method

    public int indexOf(Object o) {
        if(o == null) {
            throw new NullPointerException();
        }

        for(int i = 0; i < size; i++) {
            if(theData[i].equals(o)) {
                return i;
            }
        }

        return -1;
    } // End indexOf(Object o) method

    public boolean isEmpty() {
        return size == 0;
    } // End isEmpty() method

    public E remove(int index) {
        if(index < 0 || index >= size) {
            throw new ArrayIndexOutOfBoundsException(index);
        }

        E temp = theData[index];

        for(int i = index + 1; i < size; i++) {
            theData[i - 1] = theData[i];
        }

        size--;
        return temp;
    } // End remove(int index) method

    public boolean remove(Object o) {
        int indexOfO = indexOf(o);

        if(indexOfO == -1) {
            return false;
        }

        remove(indexOfO);
        return true;
    } // End remove(Object o) method

    public E set(int index, E e) {
        if(index < 0 || index >= size) {
            throw new ArrayIndexOutOfBoundsException(index);
        }

        if(e == null) {
            throw new NullPointerException();
        }
        E temp = theData[index];
        theData[index] = e;
        return temp;
    } // End set(int index, E e) method

    public int size() {
        return size;
    } // End size() method

    private void reallocate() {
        capacity *= 2;
        E[] newArraylist = (E[]) new Object[capacity];

        for(int i = 0; i < size; i++) {
            newArraylist[i] = theData[i];
        }

        theData = newArraylist;
    } // End reallocate() method
}

I also have the following warnings:

Note: SJUArrayList.java uses unchecked or unsafe operations.

Note: Recompile with -Xlint:unchecked for details.

What are they, and how can I eliminate them without using -Xlint?

\$\endgroup\$
1
8
\$\begingroup\$
  1. Consider the following test case:

    final SJUArrayList<String> list = new SJUArrayList<String>(4);
    list.add("a");
    list.add("b");
    list.add("c"); // theData contains the following: [a, b, c, null]
    list.remove("c"); // theData contains the following: [a, b, c, null]
    

    It's a memory leak since the theData array has a reference to a removed object, therefore the garbage collector could not deallocate the memory of the unused, inaccessible c string object. It could be serious if you have lots of lists or use this list with bigger objects.

  2. You should implement a hashCode method too:

    If two objects are equal according to the equals(Object) method, then calling the hashCode method on each of the two objects must produce the same integer result.

    The following test should pass:

    final SJUArrayList<String> listOne = new SJUArrayList<String>(4);
    listOne.add("a");
    
    final SJUArrayList<String> listTwo = new SJUArrayList<String>(4);
    listTwo.add("a");
    
    assertTrue("equals", listOne.equals(listTwo));
    assertEquals("hashCode", listOne.hashCode(), listTwo.hashCode()); // should pass
    
  3. -1 is used multiple times. It should be a named constant.

\$\endgroup\$
1
  • \$\begingroup\$ I disagree with -1 having to be a named constant. -1, 0 and 1 are pretty much exceptions to the whole "magic numbers" rule. \$\endgroup\$ – Pimgd Aug 8 '14 at 18:45
3
\$\begingroup\$
  • Would be nice if this implemented an interface to make it easier for others to use. The Iterable interface would be nice to implement so that this could be used in enhanced for loops. Ideally you would implement the List interface though, then it would feel much closer to an ArrayList.

  • The warning is being caused because you create an array of type Object and cast it to type E. Since you can't create generic arrays the only way around this is using a @SuppressWarning("unchecked") annotation. I see it done in the constructor, reallocate, and clear so you may want to create a method that will create the array for you so that you only do it once.

  • If you favor concise code you could also remove the out of bounds checks

    if(index < 0 || index >= size) {
        throw new ArrayIndexOutOfBoundsException(index);
    }
    

    If the index is out of bounds the array will throw the exception for you, this is duplicating code already in the array type.

  • I personally favor a little extra typing to shortcuts like INIT_CAPACITY, INITIAL_CAPACITY reads so much better and its only 2 extra characters.

  • Your reallocate could be shortened to one line using builtins

    theData = Arrays.copyOf(theData, capacity *= 2);
    
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Frankly, SuppressWarnings makes me kind of nervous. There has to be a better way to deal with this. Perhaps add the proper casts as needed and/or change the type of the array field to Object[]? \$\endgroup\$ – luiscubal Oct 13 '12 at 13:44
  • 1
    \$\begingroup\$ @luiscubal No, there isn't a better way because that's just the way Java generics are implemented. \$\endgroup\$ – Adam Oct 13 '12 at 13:49
  • \$\begingroup\$ Removing the out of bounds check is not always viable. For instance in the get method, the array length is probably bigger than the arraylist size. The underlying array would not necessarily throw an exception when the arraylist should. \$\endgroup\$ – Reinstate Monica Jun 16 '14 at 10:57
1
\$\begingroup\$

Bugs?

If your intention was to mimic the implementation of the real ArrayList exacly, you should allow add(null). Right now you're throwing an NPE.

Same story for indexOf(null). The real ArrayList just returns the proper index or -1 if there is no null element, but you throw a NullPointerException.

Again, maybe you intended this behaviour. But from your question I got the impression you were trying to mimic the real ArrayList as a coding exercise.

Code duplication

You have some code duplication. For instance, take a look at the two overloaded add methods. I believe you can just write the first one like this:

public boolean add(E e) {
    add(size, e);
}

Comments

You end some of your methods with a comment like this: // End add(E e) method. While this is not strictly bad it does make me think you're in the habit of writing huge methods that are so long that these comments are necessary in order to be able to comprehend what is going on. I'd say you're better off leaving these comments out, if you keep your methods small and properly indent your code, you won't need them.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.