Proposal - Use nullables instead of optionals for packet fields that may not exist on certain versions

[Jaren8r]

trafficstars

Currently, PacketEvents stores packet fields that may not exist across all supported versions as Optionals. This isn't great for several reasons:

• The intended usage of Optional is as a return value of something that needs to have a clear value of no result (such as a lookup), not as a field or a parameter (setters). IntelliJ issues a warning if you do this.
• We're unnecessarily creating wrappers around objects (and sometimes even wrappers around wrappers of primitives), which is more ram used and garbage for the gc to collect.
• It's really easy to forget to set an optional property to Optional.empty() in a packet wrapper constructor, leading to NullPointerExceptions anyways.
• It's counter-intuitive to wrap a value into an optional before passing it into a setter.
• Many more I probably can't think of.

I'm thinking that it would be better to convert all of these usages of Optional to nullable, but it would require an API breakage. I'm interested to hear thoughts on this.

[NoJokeFNA]

In fact, I would very much welcome this change. If you use them, but forget to call Optional#empty, you just get NullPointerException's, which you can just save. I would clearly be in favor of removing optionals if there isn't a good use case for them - such as with the current wrappers.

With the change, you could also start documenting the getters and setters and annotate them with @Nullable & @NotNull so that the difference is very clear and it is also very clear which method is for which server version.

[retrooper]

If possible I always try to avoid having methods for separate versions. Like bringing everything to one form. Sometimes that isn't always possible. I'll consider this.

[MWHunter]

I prefer the following scenario for the one time to use optional over null, but it breaks convention:

• Optional.empty() if not sent on a version but it exists on the version
• null if field doesn't exist on the version
• Optional with value if existing on version AND sent

This preserves information.

I'd suggest just sticking with null for readability reasons. We aren't here to convert packets between versions and any attempts to do so should be considered out of scope and avoided for maintainability reasons.

[MWHunter]

A more user proof method would be to throw an exception if the field doesn't exist on the server version like PE 1.8 did