Various improvements based on the example code

[DavidTheExplorer]

Feature Type

API Addition

Description of Feature

The idea of the library is so cool that even though I closed my Spigot account, I will contribute by providing a list of improvements based on the example code in the thread:

• Instead of calling new SocketPlayer(player); every time, provide a SocketPlayerRegistry object with a get(Player) method that would return a cached instance. I suggest using Map#computeIfAbsent.

• The Instruction class is a great place where the Builder pattern can be utilized. Consider this line: Instruction.drawText(100, 100, "Hello World", 5, TimeUnit.SECONDS) which is not clear at all - forcing us to look at the docs or just guess the roles of the 3 numbers. Instead of providing many variations of many methods, you can provide one or more builder classes: TextBuilder, RectangleBuilder etc.

• Receiving time in form of a long and TimeUnit is obsolete, Java 8 added the Duration class which is super easy to work with + saves you a method parameter.

• Cosmetic Suggestion: Exactly like how Collectors.toList() is always statically imported, modify the example to use static import: sp.sendInstruction(drawText(100, 100...));

[gmitch215]

Thanks for the suggestions!

Instead of calling new SocketPlayer(player); every time, provide a SocketPlayerRegistry object with a get(Player) method that would return a cached instance. I suggest using Map#computeIfAbsent.

I'm curious about the performance implications of caching the instance. How much of an improvement would you reasonably expect? It'd be a good idea to implement, but I'm not sure how much you would expect to save with this method.

The Instruction class is a great place where the Builder pattern can be utilized. Consider this line: Instruction.drawText(100, 100, "Hello World", 5, TimeUnit.SECONDS) which is not clear at all - forcing us to look at the docs or just guess the roles of the 3 numbers. Instead of providing many variations of many methods, you can provide one or more builder classes: TextBuilder, RectangleBuilder etc.

This is a pretty good idea. I'll keep the original methods and probably implement an InstructionBuilder class for use.

Receiving time in form of a long and TimeUnit is obsolete, Java 8 added the Duration class which is super easy to work with + saves you a method parameter.

Didn't know that. I might keep both for preference (and compatibility) sake since all I need is for milliseconds to be available.

Cosmetic Suggestion: Exactly like how Collectors.toList() is always statically imported, modify the example to use static import: sp.sendInstruction(drawText(100, 100...));

Good idea.

[DavidTheExplorer]

I'm curious about the performance implications of caching the instance. How much of an improvement would you reasonably expect? It'd be a good idea to implement, but I'm not sure how much you would expect to save with this method.

If you have a good reason to be worried about memory(or just want the best practice) you can remove the player's object from the cache when he disconnects. Anyway, creating objects when they can be cached is unnatural in OOP. Usually you don't see an immediate benefit, and sometimes you never know how much problems you avoided because you did things the right way.

This is a pretty good idea. I'll keep the original methods and probably implement an InstructionBuilder class for use.

I also thought about an InstructionBuilder but notice that I wrote "one or more" because you might need a family of builders for the different types of Instructions.

Didn't know that. I might keep both for preference (and compatibility) sake since all I need is for milliseconds to be available.

In your shoes I would've changed to Duration because the project was just announced(how many projects are going to be broken?), but it's okay if you don't want, maybe you can do that in the next major release.