Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider removal of convenience methods from Component #223

Open
dualspiral opened this issue Dec 7, 2020 · 6 comments
Open

Consider removal of convenience methods from Component #223

dualspiral opened this issue Dec 7, 2020 · 6 comments
Labels
area: api status: needs discussion An issue or PR that requires design decisions or further consensus building before it can be merged type: question

Comments

@dualspiral
Copy link

This issue is taken from SpongePowered/SpongeAPI#2275 (comment)

Also note that this is somewhat linked to #222

TL;DR: The convenience methods on Component, such as style(...), color(...) etc. encourage bad habits and should be considered for removal in the next major release, or should be renamed to make it clear that they return a new immutable object.

For this, I'm mostly copy and pasting from the issue above.


Consider the two methods

Component.text();
Component.text(String);

These return two objects with a very important difference, one is mutable, one is not. And yet, both of these are valid ComponentLikes that can be used in many areas of Adventure:

Component.text().content("Text").color(NamedTextColor.GREEN).style(Style.style().decorate(TextDecoration.BOLD).decorate(TextDecoration.ITALIC).build());
Component.text("").content("Text").color(NamedTextColor.GREEN).decorate(TextDecoration.BOLD).style(Style.style(TextDecoration.BOLD).decoration(TextDecoration.ITALIC, TextDecoration.State.TRUE)); // slighly facetious with the starting method, but it proves a point

They look pretty much the same, and yet have completely different semantics. One is created from mutable builders, one is created from immutable objects. There is nothing to tell me which one I should use (I know it should be the first one) - part of why the lack of verbs is a problem. In the first method, I create three objects, the text component builder, the style builder and the style object (a fourth will be created upon use). The second method creates five - maybe more, I haven't looked at the implementation. For more complicated structures, this is only going to get worse. There should be some way to differentiate between what happens in both of these methods, and ideally, the latter should not exist at all. But there isn't. This is bad, and I am concerned it's going to hit a lot of people, and potentially going to cause a lot of mistakes.

Some thought needs to go into what is the "right" way to do things, and these need to be promoted. I do not know what the right way to do things is as it stands.

I personally think the convenience methods should be considered for removal. If that's not desired, I suggest prefixing the methods with with, in order to clarify that these methods are not the same as the builder variants.

@kashike
Copy link
Member

kashike commented Dec 7, 2020

The reasoning for the convenience methods on Component: allow modifying without going to a full builder - for example:

Component component = this.someMethodThatReturnsComponent();
if(this.special()) {
  component = component.decorate(TextDecoration.BOLD);
}

is much nicer than:

Component component = this.someMethodThatReturnsComponent();
if(this.special()) {
  component = component.toBuilder().decorate(TextDecoration.BOLD).build();
}

@octylFractal
Copy link

I'd prefer keeping them, but with a with prefix or similar.

@dualspiral
Copy link
Author

I think that's a reasonable middle ground - it's clear(er) that a new object will be created and (I'd hope) would deter mis-using these methods on a full blown component as if it was a builder.

@A248
Copy link

A248 commented Apr 16, 2021

I'm not sure if this is still under heavy consideration, but there's an important point missed here.

Consider:

deter mis-using these methods on a full blown component as if it was a builder

On the contrary, it is probably perfectly fine to use a full component as if it was a builder.

The performance of object creation is no concern. For cases where the performance truly matters, the JIT is able to optimize away the allocation. Some other methods, like appending child components, create further objects behind the scenes. There's object creation happening all over, but it's simply not an issue, and accordingly, not a reason to consider one aspect of the API an abuse.

@octylFractal
Copy link

octylFractal commented Apr 16, 2021

the JIT is able to optimize away the allocation

[benchmarks needed]
The JIT is good but not always fantastic about avoiding allocations. Though I think in the case of Components, it's unlikely to run up against GC pressure in practice anyways.

@astei
Copy link
Contributor

astei commented Apr 16, 2021

the JIT is able to optimize away the allocation

This is highly specific to the use case (it relies on a lot going right in ways that are hard to predict). I doubt Components would be the source of GC pressure in an application except in the most extreme cases (and in that case, your application needs to take measures against that).

@zml2008 zml2008 added the status: needs discussion An issue or PR that requires design decisions or further consensus building before it can be merged label Nov 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: api status: needs discussion An issue or PR that requires design decisions or further consensus building before it can be merged type: question
Projects
None yet
Development

No branches or pull requests

6 participants