Someone breaks my sorting of a list - now which approach to choose: return unmodifiable List or a new List altogether?

Question

I'm using lists on a jsf web server to e.g. access the data model from web pages. The access to these lists is done from various other places as well though (web services, tools).

There is a piece of code which gets broken by someone resorting the list I return. With someone I am talking about someone in my developer team - we are the only ones using this code. I have roughly 300 references on this function and it could be performance relevant to do the fix nicely:

The list can be anywhere from 1 - 10'000 entries and commonly I will have maybe 10-100 such lists. In reality I will probably have very often around 20 lists with each having 8 entries - so not such a big deal. But I can have more sometimes

I am btw talking about a function something like this:

public List<MyObject> getMyObjectList() {
    if (this.myObjects== null) {
        myObjects = new ArrayList<MyObject>(myObjectsMap.values());
    }
    return myObjects;
}

Now I could of course do the return like this:

    public List<MyObject> getMyObjectList() {
        if (this.myObjects== null) {
            myObjects = new ArrayList<MyObject>(myObjectsMap.values());
        }
        return Collections.unmodifiableList(myObjects );
}

But this will break, eventually at several places in different projects / applications. It would imho be the cleanest to return unmodifiable, add javadoc - and fix everything which breaks. But :-D this is work. I'd probably have to test roughly 10 applications.

On the other hand I could just return a new list, e.g.

public List<MyObject> getMyObjectList() {
    return new ArrayList<MyObject>(myObjectsMap.values());
}

Which is no to little work - but what about the performance issues with this? Other than that - if someone is deleting stuff from the list I returned, it will silently break the application.

So: What is the performance issue? Is it an issue?

What would you do?

Answer 1

What would you do?

If I understand you correctly, this is a production library used in several applications. And, like it or not, the de-facto contract for getMyObjectList() is that the user can sort the list without getting an error or exception.

I would immediately change this method and return a defensive copy:

// good idea
public List<MyObject> getMyObjectList() {
    return new ArrayList<MyObject>(myObjectsMap.values());
}

You have now fixed the problem where someone is sorting your internal collection and you have not broken your contract. In fact you can even update the Javadoc and tell the user that they can do whatever they want with the copy.

This may or may not cause a performance problem. Remember, the objects in the collection are not being copied - they are still being shared. You are just creating a new array list and whatever internal objects it needs to keep track of the objects.

If it turns out that these copies are causing a performance problem, then you can consider enhancing your class to include a read-only cache of your internal collection. To access this you must give the method a new name - ex getMySharedObjectList and you can gradually update client code to use this new method as performance needs require.

But don't do it this way. I think this method is particularly bad:

// bad idea
public List<MyObject> getMyObjectList() {
    if (this.myObjects== null) {
        myObjects = new ArrayList<MyObject>(myObjectsMap.values());
    }
    return Collections.unmodifiableList(myObjects );
}

You have created a situation where it is very easy for myObjects to get out of sync with myObjectsMap. (What happens when an item is added to myObjectsMap after someone called getMyObjectList?) At the same time, you are making a copy of the list every time someone calls the method. So you just gave up whatever theoretical performance gains you had in the first place.

Anyway, good luck. Hope this helps.

Answer 2

If you can afford to test the applications, I'd go with the unmodifiableList. It will save you from other related issues in the future.