up vote 4 down vote favorite

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 maybe are not comprehensive. So I decided to ask help from CodeReview

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 to eliminate them without using -Xlint?

share|improve this question
feedback

2 Answers

up vote 6 down vote accepted

  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.

share|improve this answer
feedback
up vote 4 down vote

  • 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);
share|improve this answer
1  
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[]? – luiscubal Oct 13 at 13:44
1  
@luiscubal No, there isn't a better way because that's just the way Java generics are implemented. – codesparkle Oct 13 at 13:49
feedback

Your Answer

 
or
required, but never shown
discard

By posting your answer, you agree to the privacy policy and terms of service.

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