GetPlayer

[ghost]

Dude. server.getPlayer()s are really resource-demanding.

It needs to trawl through every player and find the right one. Just store a player variable.

Also, why throw an exception? Just return null, and then do a npe check when you call the method.

Finally, it's perfectly fine to store Player variables in lists so long as you do it right. It's actually worse to do "Bukkit.getPlayer()" or "server.getPlayer()" over and over.

[storm345]

If you want this then show me a working PR with the player object used and attach a compiled version of the modified plugin to test. If there are no exceptions and the system doesn't break any features, then I will merge it.

[ghost]

I'm referring to the PR that niccholaspage made. Player objects are fine.

I know that all around BKForums there are warnings about Player Objects and HashMaps, but that's only for the newbs who don't clear the hashmaps afterwards. Java's GC will deal with all that fine.

I don't 'want' you to change anything. I want you to know that it's OK to use player objects.

If anything, using 'getPlayer()' is worse over and over because on a large server, you need to go through every single EntityPlayer(NMS) and see if the name matches.

[storm345]

Actually never mind, I can easily implement the requested change with the use of the User class and storing the player there... That way all the maps, etc.. Can stay as User or String (so as to not break half the plugin) while having the same efficiency improvement you request.

[storm345]

Okay well from one of the heavy Bukkit contributers (http://forums.bukkit.org/threads/common-mistakes.100544/), not to mention the getPlayer call is ONLY done when explicitly necessary and the resource use is lowered there. Maybe when I have time I will test out storing it and check for any errors. If I see ANY then I won't change. (If the API writers warn against it then I highly doubt it's a good idea.)

[ghost]

Look here

https://github.com/Bukkit/CraftBukkit/blob/master/src/main/java/org/bukkit/craftbukkit/CraftServer.java#L346

This is the CraftBukkit source.

You'll see that calling "getPlayer(name)" searches through getAllPlayers(), which in turn goes through all the EntityPlayers in MinecraftServer.getPlayerList().players"

Yes. Calling getPlayer(name) once is fine. Calling it every few ticks because of all the players joining and leaving a game and whatnot is not.

As long as you know where objects are getting stored, and where they aren't, you can safely store Player variables and clear them out when needed.

Also, while I'm here, instead of using a BitLy shortner every time, just store the link.

[ghost]

And check out this quote. Pretty much what I said.

I'm a bit ambivalent about this one. What sort of problems, exactly? The one that springs to mind is that if a player logs off, you have a potential memory leak and hanging references, so you need to listen for the PlayerQuitEvent and perform any necessary cleanup.

Storing the player name, on the other hand, is less prone to memory leakage etc., but when you need a Player object from the stored name (as you surely will sooner or later), you have to call getServer().getPlayer() or getServer().getPlayerExact(). These are slow methods, especially for busy servers. One might naïvely believe it's a just doing a HashMap lookup, but no: both methods do an array copy, followed by a linear search and compare of all logged-in players. Calling either method from e.g. a PlayerMoveEvent handler on a busy server is asking for lag.

https://github.com/Bukkit/CraftBukk.../org/bukkit/craftbukkit/CraftServer.java#L306

So I'm really not convinced anymore that storing player names all the time is such a good idea...

[storm345]

I think I will shorten the link on plugin enable as that's a good idea... Anyway... If you REALLY want the Player object then go into the User class and store the player. Then on disconnect change the class to have 'getOnline()' to false and set Player to null or whatever... Then submit as PR. But... The User object cannot be removed by methods outside of playerOut(). One final thing.. Does Bukkit revalidate Player objects on /reload and will the stored object remain correct with the users details? (Is it a copy of the actual Player?)