guava
guava copied to clipboard
google
Add Immutable*Array.reverse()
ImmutableIntArray and its counterparts are a nice abstraction over primitive arrays that do not only offer the benefit of immutability, but also provide the nice ability to create a sub-array view without copying it, which is not possible with primitive arrays.
In a similar manner it would be nice if these classes would provide a reverse() method that returns a reversed view.
Precedent for this is ImmutableList.reverse().
My use case is existing code that uses ImmutableIntArray where I want to optionally allow reversing the order of iterations. Currently this needs to be done by dumping the ImmutableIntArray into an array, call Ints.reverse(), and reconstruct an ImmutableIntArray.
The idea is reasonable, but I'm not sure the method would be used very often. I'm a little shocked to discover that in the internal Google codebase, Ints.reverse() has zero usages.
Hm, maybe because it is not obvious to find?
Interestingly, in the last two days I found already two cases where I would use such a method in our code base: one where I indeed want to reverse an arbitrary ImmutableIntArray because I want to call a certain library method with the reversed data in order to let it try again with a different strategy after a failure (here I now have exactly the 3-step code described at the end of my issue) and one case where I would like to do ImmutableIntArray.copyOf(IntStream.range(0, someList.size())).reverse() (here I now have a builder and a manual for loop). And we don't actually use ImmutableIntArray that much.
I could see an argument that Immutable*Array should mirror ImmutableList except when we have a specific reason that a method is dangerous there.
(Then again, I wish we had named ImmutableList.reverse "reversed" instead, just to extra emphasize that it's not a mutator. Or really, since ImmutableList is obviously not mutable, the goal would be to teach users to subconsciously recognize that the "-ed" suffix implies "view." Anyway, we could question whether to replicate that "mistake" here. But the method on ImmutableList will be used far more often, so I can't imagine that the name of the method on Immutable*Array is going to be the one to teach anyone a valuable lesson :))
I was thinking about that too. Feels bad to replicate a naming mistake; feels bad to be inconsistent. I think 2010 was before we had a defined API Review process and the subject of the name just never came up on the code review thread.
reverse is totally wrong tho.
We might not have questioned the name, given preexisting Iterables.reverse from way back in 2006. And of course Iterables.reverse is even worse (since some Iterable types are mutable), and we made it worse still with Lists.reverse (since List has mutation operations defined on that type) :)
I also prefer reversed and used reverse only because of the existing method.
I wonder whether all these methods should be marked as @Deprecated and methods named reversed be added?
However, it does not stop with reverse, there is also Iterables/Sets.filter, where filtered would be logical (and same for transform). But filter is consistent with FluentIterable and Stream, where the convention is to have all of the methods with just the verb although they could also be seen as returning views. Maybe the consistency with FluentIterable and Stream is an argument to keep using names like reverse in the Java world instead of starting to use -ed for collection views?
The only -ed methods for collections that I remember now are Collections.checked*/synchronized*, where the non--ed form would be even more confusing.
For comparators the situation is inconsistent: Collections.reverseOrder(Comparator) but Comparator.reversed().
The idea is reasonable, but I'm not sure the method would be used very often. I'm a little shocked to discover that in the internal Google codebase,
Ints.reverse()has zero usages. Hm, maybe because it is not obvious to find?
Well, in our codebase the Ints class has 12 times as many usages as ImmutableIntArray. So...
As always, we appreciate the offers of help, but the team would first need to decide that this is worth having. The actual effort of writing/testing the code isn't the hard part. I'm sorry that making that decision hasn't been a high priority for us, but this will probably sit in the state it's in a bit longer. :-/
The absence of ImmutableIntArray.reversed() may someday look more surprising if SequencedCollection lands with its reversed() method. (Not that ImmutableIntArray is a Collection, much less a SequencedCollection.)
I do see a team that implemented and used this for a couple of our types, plus a team that implemented it but didn't end up using it, plus a team that iterated over an input object in reverse order so that it could build an instance of one of our types in reverse order. But that's still only ~3 callers....
Sure, over time, familiarity with SeqColls might lead more users to "expect" to find reversed() on these classes too. That's going to be a very slow effect though, and at some point Valhalla's going to enable ImmutableList<int> and make this whole class obsolete.
At this point I think it's fair to say that the case is not "clearly compelling" enough. Reopen if something changes.