views:

182

answers:

9

I have come across a class that has an immutable property:

MyObject[] allObjs

The property is initialized like this:

List<MyObject> objs = createAllMyObjects();
allObjs = objs.toArray(new MyObject[objs.size()]);

When it is exposed through the accessor, it's done as a List:

public List<MyObject> getAllMyObjects() {
    return Collections.unmodifiableList(Arrays.asList(allObjs));
}

Why would a programmer do this? Is there a benefit that I don't know about?

Performance is not a concern, as the array of objs will only ever number in a few dozen elements.

It seems that we are going round and round in circles.

The class is a sort of factory, so it's got a private constructor and exposes only static methods (not sure if this could be a reason for the madness).

edit

I guess that my question is really, "Why not just use an internal List<MyObject> allObjs instead of MyObject[] allObjs, and return the Collections.unmodifiableList(allObjs)?" as this would accomplish the same thing.

A: 

I don't see any advantage in using arrays instead of lists. Lists are far more comfortable. Maybe the guy just likes working with arrays:)

Petar Minchev
A: 

I think the reasoning behind this could be that the array is not modifiable. However, you could keep the internal list as a List and just return a copy of it in the getter, that way the internal list would still not be modifiable from outside.

Or use the "unmodifiable" list returned by Collections.unmodifiableList()...

In this case I don't see any performance benefit either, because the conversion back and forth anyway takes some cycles.

fish
"The array is not modifiable". What does this mean? The only unmodifiable arrays are those with zero length!
Dimitris Andreou
You are of course right, Dimitris, it is possible to remove and replace items from an array, but you can't add anything or change the size. And that's why I think the original coder has decided to use the array instead of a List.
fish
A: 

This way the collection is unmodifiable, i.e. the caller can not modify the member array. There is no way to do this with an array so the coder copies it into a list.

openCage
The same could be achieved by just returning `Collections.unmodifiableList(myList)`. No need to go with an array for that reason.
Joachim Sauer
But that returns a list and not an array.I think the original coder wanted to use an array and prevent callers to modify it, but there is no 'unmodifiableArray'.
openCage
See my edit: if the interface says it must be a List, then is there any reason to use an array internally.
Tim Drisdelle
+1  A: 

It seems that the writer was intent on returning an immutable copy and has done so redundantly. I'm assuming that a List was chosen as the return type so a clone could be easily created with the MyObject type.

Melvin
It is as easy to "clone" (or literally, clone()) an array as well as a List.
Dimitris Andreou
+1  A: 

There is a good reason not to return a reference to the internal MyObject[] attribute. Via the reference changes can be made to the array by the caller.

If there is no reason why the result has to be a List, you can also return a copy of the array:

public MyObject[] getAllMyObjects() {
    MyObject[] result = new MyObject[allObjs.length];
    system.ArrayCopy(allObjs, 0, result, 0, result.length);
    return result;
}

Update, after addition to the question: You are correct that if the internal attribute is changed into a List, wrapping it to be unmodifyable is easier.

The reason why the class uses an array instead of a List might be performance. If the array does not change (much) during the lifetime of the object and is iterated a lot, an array has less overhead than a List does.

If performance is the reason for using the array, you could prevent extra object creation in the getter by storing the List for the sole purpose of being returned, i.e.:

allObjsList = Collections.unmodifiableList(createAllMyObjects());
allObjs = allObjsList.toArray(new MyObject[allObjsList.size()]);

use the allObjs array like before and return the stored list in the getter:

public List<MyObject> getAllMyObjects() {
    return allObjsList ;
}
rsp
Actually `Collections.unmodifiableList(Arrays.asList())` avoids copying the data. It only creates 2 small wrapper objects.
Joachim Sauer
@Joachim, according to the question the class internally works with an `MyObject[]`, not a list, hance copying ino a new array to return.
rsp
Yes, I know, I just wanted to point out that "returning a copy of the array" is not the only alternatve to "returning the array" and that "returning a unmodifiable List wrapper" is actually cheaper and provides a richer interface to the caller than copying the array.
Joachim Sauer
+6  A: 

The only reason I see is "fail early in case of misuse" (if this design choice was not random and has a meaning, the meaning is he trusts very little his particular clients).

MyObject[] is reified, and checked at runtime, while List<MyObject> is not. One can sneak non-MyObject references in the latter (he'll get unchecked warnings at compilation of course), which will fail at some undefined future point with a ClassCastException, but he cannot do the same with MyObject[] - that will fail immediately at the point of misuse.

Note though that if the clients are prebuilt binaries that was using this API before it was generified, then nobody got unchecked warnings, so if this code is trying to migrate code used by pre-generics clients, it makes sense.

(Another way to achieve the same result would be using List<T> and also requiring Class<T>, so that the latter can offer the runtime type checks. But that would require for example another constructor parameter - existing clients wouldn't automatically take advantage of that. *Update: * This basically describes the API call mentioned in the first comment by Sauer, so one does not have to reinvent this, just use that one :) ).

Dimitris Andreou
That's a good point! However, you could provide a `checkedList` to get fail-fast behaviour for genericized Lists: http://java.sun.com/javase/6/docs/api/java/util/Collections.html#checkedList(java.util.List,%20java.lang.Class%29
Joachim Sauer
Agreed. If the original poster wants to sanitize the code without affecting its (even accidental) semantics, switching to checkedList should be a safe route.
Dimitris Andreou
Thanks for the comments and the answers.
Tim Drisdelle
A: 

In the Java Generics book, the author actually say that since we have Generics, we should use collection exclusively and should no longer use array. Personally I now tend to stay away from arrays because collections helps me remember my intention of using a collection of things eg. sets vs list as oppose to array which doesn't really tell me anything about the data.

Also there isn't anything thing that I cannot do with collections that I cannot do with an array with the exception of maybe a fast copy (System.arraycopy())

Chuk Lee
+2  A: 

There may be a few reasons for this:

  • They didn't know what they were doing. I'm working in code now that appears to have been written by someone new to Java and new to programming. It's unfortunately far too common to come across poorly designed code than to find good, clean, well-maintained code written by someone with a solid understanding of the platform. This is partly because of the next point.
  • The List changes were patched without wanting to make wholesale changes. If the rest of the code uses the array, someone may not have wanted to bother with making all the other changes.
  • While Collections are the more OO/Java way to do things, if the list is static, an array is actually "easier" to work with since they can still be iterated with for(:) like a Collection but accessed with [] instead of get(). Personally, not a good enough reason to use arrays but maybe that's just me.
  • Maybe they wanted it to be clear that the length of the data is final. If they had just used an unmodifiable list, a List would still be the means for referring to the data and anyone not paying attention to the rest of the (hopefully commented) code might try to add to the List which would result in a run-time UnsupportedOperationException. Making it an array makes it's usage clear.
nicerobot
A: 

Let me answer with a joke:

The new Jewish bride is making her first big dinner for her husband and tries her hand at her mother’s brisket recipe, cutting the ends off the roast the way her mother always did. Hubby thinks the meat is delicious, but says, “Why do you cut off the ends—that’s the best part!” She answers, “That’s the way my mother always made it.

The next week they go to the old bubbie’s house, and she prepares the famous brisket recipe, again cutting off the ends. The young bride is sure she must be missing some vital information, so she asks her grandma why she cut off the ends. Grandma says, “Dahlink, that’s the only way it will fit in the pan!”

While we certainly can find some arguments to justify a particular decision, in practice this rarely makes any difference and often there's no intent behind decisions like this.

Use the simplest and easiest to read option. Readability is often more important than cleverness.

Jan Soltis