CODEWITHSUNDEEP
javaarrays
Klausos Klausos
Klausos Klausos

Reputation: 16060

Delete some elements from the array

I need to delete elements from the array points. This is how I do this. The problem is that pts.length is always the same, and the removed elements have the value null. Therefore at some moment I receive the error message java.lang.NullPointerException.

for (int i = 0; i < points.length; i++) {
    int ind = r.nextInt(pts.length);
    TSPPoint pt = points[ind];
    pts = removeElements(points,ind);
    solPoints[i] = pt;
    System.out.println(pts.length);
}

private static TSPPoint[] removeElements(TSPPoint[] input, int ind) {
    List<TSPPoint> result = new LinkedList<TSPPoint>();

    for(int i=0; i<input.length; i++)
        if(i != ind)
            result.add(input[i]);

    return (TSPPoint[]) result.toArray(input);
}

Upvotes: 1

Views: 114

Answers (3)

tobias_k
tobias_k

Reputation: 82949

What your code seems to (be supposed to) do is to remove random elements from the original array of points and append them to the pts array, i.e., create a permutation of points.

If this is the case, I suggest converting your array to a List and using Collections.shuffle.

Upvotes: 1

Oleg Sklyar
Oleg Sklyar

Reputation: 10092

Wow, for deleting every element you recreate the rest of the array in a LinkedList, which you then turn into an array... The performance of this code, both in time and space, is terrible and so is readability, maintainability and testability.

Why not drop using arrays all together?.. convert points into an ArrayList and use remove(index) directly on that list and use an iterator of the llist to remove multiple elements as you iterate through?

Upvotes: 1

T.J. Crowder
T.J. Crowder

Reputation: 1075567

@nrathaus has found the bug for you. It's simply that you've got your arrays confused (you're passing points into removeElements, but using pts everywhere else).

But if memory churn is being an issue, there's a much more efficient way to implement removeElements, using System.arraycopy rather than a temporary LinkedList.:

private static TSPPoint[] removeElements(TSPPoint[] input, int ind) {
    TSPPoint[] rv;

    if (ind >= 0 && ind < input.length) {
        // New array will be one smaller
        rv = new TSPPoint[input.length - 1];
        if (rv.length > 0) {
            // Copy the bit before the element we delete
            if (ind > 0) {
                System.arraycopy(input, 0, rv, 0, ind);
            }

            // Copy the rest
            System.arraycopy(input, ind + 1, rv, ind, input.length - ind);
        }
    }
    else {
        // No change
        rv = input;
    }

    return rv;
}

Mind you, if you're doing this a lot, creating and releasing all of these arrays may not be ideal. Using a List throughout may be better.

Upvotes: 1

Related Questions