represent getOnlinePlayers as a collection

[sunmisc]

Small changes in the API (TabAPI) Not the most efficient (also not the most pleasant) - working with arrays, we will talk about the TabPlayer[] getOnlinePlayers() method. There are several reasons why this is bad 1) We are giving out a pre-constructed array of users, let's say I need to get a player by predicate, instead of giving out all players at once, we can ask for the "next" example is Iterable<TabPlayer>, or Stream<TabPlayer>.. (The current implementation just returns the field, but conceptually this isn't very correct.) 2) The current implementation copies an array for each modification (addPlayer, removePlayer), which is also bad. 3) The user gets an unsafe array that he can modify and break everything, but if you clone the array on every read, optimization 2) is useless.

As is, working with an array is bad in this case.

What I suggest: Mark TabPlayer[] getOnlinePlayers() as @Deprecated.

and add a new method Collection<TabPlayer> onlinePlayers().

[NEZNAMY]

Why implementing it in both TabAPI and TAB classes? Players are converted into an array to avoid creating new iterator in every loop, although it probably should not be exposed in API, that's true. Why returning Stream instead of a collection? You can then use map values for that. You cannot iterate over a stream.

[sunmisc]

Why implementing it in both TabAPI and TAB classes? Players are converted into an array to avoid creating new iterator in every loop, although it probably should not be exposed in API, that's true. Why returning Stream instead of a collection? You can then use map values for that. You cannot iterate over a stream.

Here iterator allocation is cheap ( the developers are working on it at least)

And the current state does not allow to work correctly If the user does: getOnlinePlayers()[0] = null To fix this problem cloning will be much more expensive

[NEZNAMY]

Yes, that's why I suggested returning a collection, which will return map values.

[sunmisc]

Yes, that's why I suggested returning a collection, which will return map values.

or so it seems. Wrapping the collection in Collection.unmodifiableCollection(map.values()) would also be very cheap.

[sunmisc]

Yes, that's why I suggested returning a collection, which will return map values.

or so it seems. Wrapping the collection in Collection.unmodifiableCollection(map.values()) would also be very cheap.

Another option is Iterable<TabPlayer>

[NEZNAMY]

Map value set is already immutable.

[sunmisc]

Map value set is already immutable.

Returns a view of the values contained in this map. The collection is backed by the map, so changes to the map are reflected in the collection, and vice-versa. The collection supports element removal, which removes the corresponding mapping from this map, via the Iterator.remove, Collection.remove, removeAll, retainAll, and clear operations. It does not support the add or addAll operations.

[NEZNAMY]

Interesting, when I did something like that by accident I got an exception. Why would you modify all internals too? It works totally fine. Just add 1 abstract method to the API and implement it.

[sunmisc]

Interesting, when I did something like that by accident I got an exception. Why would you modify all internals too? It works totally fine. Just add 1 abstract method to the API and implement it.

I removed copying during the modification, so to not get a regression on getOnlinePlayers (which now copies), I replaced everything with collection

[NEZNAMY]

That's an internal method, keep it the way it is. Just make it deprecated in API and add a new one. I might just add it myself.

[sunmisc]

That's an internal method, keep it the way it is. Just make it deprecated in API and add a new one. I might just add it myself.

I guess that's it