Add OfflinePlayer#ifOnline

[Leguan16]

This PR adds a ifOnline method to OfflinePlayer. I think its self-explanatory what it does.

[montlikadani]

We could save some resource by caching getPlayer into a variable as isOnline invokes getPlayer and performs a not-null condition.

Suggested change

	+ if (this.isOnline()) {
	+ consumer.accept(this.getPlayer());
	+ }
	Player player = this.getPlayer();
	if (player != null) {
	consumer.accept(player);
	}

and do the same in CraftPlayer

[Leguan16]

good point.

[emilyy-dev]

This does not really seem to provide much value when you can just do

OfflinePlayer op = ...
if (op.getPlayer() instanceof Player player) {
  // ...
}

at best you will save one line for one line, every other case isn't really excusable

[molor]

Maybe using isConnected instead of isOnline will be better? But then I think you'll have to rename the method to ifConnected... Or, just add both methods...

[Leguan16]

This does not really seem to provide much value when you can just do

OfflinePlayer op = ...
if (op.getPlayer() instanceof Player player) {
  // ...
}

at best you will save one line for one line, every other case isn't really excusable

I mean I think that is the nature of if methods. See Optional#ifPresent for example and people still use it.

[mbax]

I mean I think that is the nature of if methods. See Optional#ifPresent for example and people still use it.

Optional's whole purpose is being potentially present, though. It can't be cast from Optional to Definitely

[emilyy-dev]

This does not really seem to provide much value when you can just do

OfflinePlayer op = ...
if (op.getPlayer() instanceof Player player) {
  // ...
}

at best you will save one line for one line, every other case isn't really excusable

I mean I think that is the nature of if methods. See Optional#ifPresent for example and people still use it.

I don't see how it makes sense to compare OfflinePlayer with Optional, one is meant to be a programming safe-guard for language/library design, OfflinePlayer is just a data bag, not an Optional<Player>. Calling it "the nature of if methods" asks to litter every piece of "maybe X" data, Map.ifContains(k, Consumer<V>), List.ifContains(int idx, Consumer<E>), Inventory.ifNotEmpty(int idx, Consumer<ItemStack>) etc

Optional.ifPresent is used because it's the better alternative to isPresent/isEmpty + get until deconstrucion patterns arrive (if (optionalString instanceof Optional.of(String str))),

[Leguan16]

I must admit that I did not have your way in mind for that case. But I don't think that this method is useless at all either and certainly not redundant.
You said that this would save one line but especially if my code just needs one line its super handy to have such a method.

For example you generally work with OfflinePlayer and write a friend system and someone sends a friend invite it looks better (my opinion) if you use

OfflinePlayer op = ....

op.ifOnline(player -> player.sendMessage("x sent you a friend request"));

// ...

instead of

OfflinePlayer op = ...

if (op.getPlayer() instanceof Player player) {
  player.sendMessage("x sent you a friend request");
}

// ...

[electronicboy]

it's yet another patch that we have to maintain when we've already got far too many patches for githubs folder to manage, which adds very little other than saving an line or two in code, with a method which extends the usage of an API method which quite frankly the implementation of sucks and probably should not be expanded too much further

[Leguan16]

I get the second part of your message, but I don't understand the first part. As far as I know, once the repo restructure happens there will be a patch per class, so it won't be another patch and you might also have too many patches (I don't know how many classes Paper modifies) for github and since I don't add a new class i also won't add a new patch. But starting to argue with the fact that "githubs folder can't manage it" or "the maintenance effort of the patch", really?

But as I already said, if you generally don't want to expand the OfflinePlayer API because its implementation sucks it's a fair argument which I understand.

[electronicboy]

Yes, really. We use GitHub and manage the project on GitHub, the fact that we cannot even view the patches folder is a pretty hefty deal, and until we hard fork, that is going to be an annoying problem to contend with. With the new layout, patches will be organized into a folder tree, so that we don't have over 1000 patches in a folder, and then we'll be able to manage it.

In that time, either patches should be merged into other patches, or they need to further justify their worth to the project, and this, I generally cannot see what value it adds especially given its reliance on a mechanism that does not accuratly represent what people expect that it's intended to mean, if you want to validate if you can actually send a message, just seeing if getPlayer returns a value is generally not enough. isOnline does not promise that a player is actually connected and able to recieve anything, all it infers is that a player instance is still registered, the only times where this method makes sense is when you literally do not care if a player is actually online or not, you just care if they're registered, which imho is not a general contract I want to be expanded upon within the API.

This also does not even need a server patch, for that matter, all of what this does could be implemented inside of an API patch.

[DerEchtePilz]

Not to be rude or something but I feel like this is a "I just want to contribute something" kind of deal. Is getting rid of like two lines of code worth the maintenance?
I also don't think it's worth adding something like this since as previously mentioned you can already write code that only executes if the player is online.
But, the decision is not up to me, and again, I am sorry if I came along rude with this first sentence.

[kennytv]

This is getting a bit out of hand, so here's some closing words: As emily said, the benefit to this is relatively small. Patch formats aside, adding such helper methods to API for saving the braces and line breaks of an if statement in fairly niche use-cases isn't worth having another method you have to scroll through. Appreciate the thought anyways

[Leguan16]

Not to be rude or something but I feel like this is a "I just want to contribute something" kind of deal.

No. It is not just a "I just want to contribute something" case. I have already said above that I just wasn't aware of the instance of check and later I expanded my argumentation with a use case. I already admitted that, if you don't want to expand this particular API because its implementation sucks, is another good reason.

isOnline does not promise that a player is actually connected and able to recieve anything...

Well that would be easily changeable with using isConnected instead of isOnline or not?

This also does not even need a server patch, for that matter, all of what this does could be implemented inside of an API patch.

Yeah, I also thought about that after I opened the PR and planned to change it once I got more feedback about it's general existence.

But as the majority of people here thinks that this is a redundant addition and some even think it's just to farm contributions I might as well close it.

Thank you for the feedback anyways.

Edit: Sorry Kenny, didn't see your message. Thanks for your input though.