2
\$\begingroup\$

I have a solution for a circular array. How can I improve this?

package oopdesign.CircularArrayNes;

import java.util.Iterator;
import java.util.Spliterator;
import java.util.function.Consumer;

public class ArrayIterator<T> implements Iterable<T> {

    @Override
    public Iterator<T> iterator() {
        return null;
    }

    @Override
    public void forEach(Consumer<? super T> action) {

    }

    @Override
    public Spliterator<T> spliterator() {
        return null;
    }
}

package oopdesign.CircularArrayNes;

public class CircularArray<T> {

    protected T[] array;
    protected int limit;
    protected int pointer;
    protected int index;


    protected CircularArray(){
        limit = 5;
        T[] array = (T[])new Object[limit];
        pointer = 0;
        index = 0;
    }

    public void add(T data){
        index = getCurrentIndex() + 1;
        array[index] = data;
        pointer++;
    }

    public int getCurrentIndex()
    {
        return (pointer) % limit;
    }

    public void delete(T data){
        // 4 5 9 11 1
        // delete 9
        int deleteIndex = findIndex(data);
        if (deleteIndex == -1) return;
        for(int i=deleteIndex;i< array.length-1;i++) {
            array[deleteIndex] = array[deleteIndex+1];
        }
        pointer-- ;
    }

    private int findIndex(T data) {
        for(int i=0;i< array.length;i++) {
          if (array[i] == data){
              return i;
          }
        }
        return -1;
    }

}
\$\endgroup\$
2
\$\begingroup\$

The Iterator is a second stage provision for API users. As such I will not comment on it.

The package name should not contain capitals. It is only a convention, but for java it is maintained relatively strict in the community, due to the hundreds of libraries.

Using T[] or rather Object[] requires some disregard to typing. I solved that by passing the class of T: Integer.class or such. Alternatively one could use ArrayList<T> instead of T[]. Then there is full generic typing.

protected is a dubious choice for such a container class, but maybe a next task would be to add functionality in the form of a child class.

Now to the critics:

  • limit should rather be a parameter in the constructor, made two constructors, one with a default limit. As limit is redundant, equal to array.length you need not have it as field.
  • You can use final for unchanging fields.
  • index and pointer are no-names, often unsuited for fields.
  • No display of really using circularity, start index and end index running around. Delete in the middle is atypical for a circular array, should not go from 0 to length, and should actually delete 9 in 3 9 5. Here I represented circularity by a size to distinghuish empty from full, and a start index consumeIndex and an end index produceIndex. Modulo array.length is needed.
  • findIndex used == but the Object wrappers are hideous: Integer.valueOf(3) == Integer.valueOf(3) (internal cache upto 128) but Integer.valueOf(300) == Integer.valueOf(300). So use equals.

So (not guaranteeing correctness):

import java.lang.reflect.Array;

public class CircularArray<T> {

    public static final int DEFAULT_LIMIT = 5;
    private final Class<T> elementType;
    private final T[] array;
    private int consumeIndex = 0;
    private int produceIndex = 0;
    private int size = 0;

    public CircularArray(Class<T> elementType) {
        this(elementType, DEFAULT_LIMIT);
    }

    public CircularArray(Class<T> elementType, int limit) {
        this.elementType = elementType;
        array = (T[]) Array.newInstance(elementType, limit);
    }

    public void add(T data) {
        if (size == array.length) {
            throw new IllegalStateException("CircularArray is full");
        }
        array[produceIndex++] = data;
        if (produceIndex >= array.length) {
            produceIndex = 0;
        }
        size++;
    }

    public void delete(T data) {
        // 4 5 9 11 1
        // delete 9
        int deleteIndex = findIndex(data);
        if (deleteIndex == -1) {
            return;
        }
        for (int index = deleteIndex; (index + 1) % array.length != produceIndex; ++index) {
            array[index] = array[(index + 1) % array.length];
        }
        produceIndex = (produceIndex - 1 + array.length) % array.length;
        --size;
    }

    private int findIndex(T data) {
        for (int i = 0; i < size; i++) {
            int index = (consumeIndex + i) % size;
            if (array[index].equals(data)) {
                return index;
            }
        }
        return -1;
    }

}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ It seems me there is a typo array in the produceIndex of method delete. I have a question, could you explain me for Integer comparison about internal cache up to 128 ? Thanks in advance for your time. \$\endgroup\$ – dariosicily Apr 19 at 15:56
  • \$\begingroup\$ The JVM has a (configurable) cache of Integer.valueOf of -128 to 127, also used by auto-boxing. These 256 Integer objects derived from autoboxing are identical (==) objects. An other int value can lead to several Integer objects instances with the same value (!=). Will correct \$\endgroup\$ – Joop Eggen Apr 19 at 20:19
  • \$\begingroup\$ I got it, thank you for your answer. \$\endgroup\$ – dariosicily Apr 20 at 6:33

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.