Discussion and thoughts on naming conventions for `Builder`s (and `Factory`s)

Context

I found some inconsistencies regarding Builders and Factorys in some mappings I am working on and some more by doing a bit of grepping, as well as having to spend more time than probably necessary figuring out what Builder methods were doing, but I'd like some discussion from the community before I start rgrepping them all away, creating resistance by changing the status quo, and wasting everybody's time.

Proposal Summary

Update CONVENTIONS.md:

[ ] Don't name Builders as Factorys
[ ] Don't put create methods in Builders
[ ] Don't use Builder#property for anything but simple setters.
[ ] Use withProperty for builders whenever possible, to increase readability.
[ ] Naming convention for array-like property setters in Builders.

`Factory`

There are quite a few classes named Factory that are actually Builders.

I propose adding a section to CONVENTIONS.md for this explicitly denouncing naming a class Factory when one of its methods returns a Factory (Thought: Would a Factory returning itself be a FactoryFactory?).

`ThingBuilder#create`

The naming conventions contain a paragraph about naming constructing functions create.

This is fine, however, in the case of Builders I feel like the name build is more appropriate. A ThingBuilder#create might be expected to be "creating a ThingBuilder", whereas a ThingBuilder#build pretty unambiguously "builds a Thing"

I propose adding an explicit convention for Builders to use build, both to prevent people from adding create methods to builders in the future, and to standardize the naming convention for them.

Note that GsonBuilder, which is used by Minecraft, (unfortunately) uses create.

`Builder` setters

The naming conventions contain a section recommending naming setter methods in Builders as the name of the property they are setting, without any prefix. This works great for builders of simple struct-like types, but quickly falls apart for more complicated Things.

As a solution, I propose using Thing Thing#withProperty(Property property);, which CONVENTIONS.md also recommends, to allow classes the freedom of adding more complicated setters without creating inconsistency (Where either some methods have no prefix and some do, or even worse, where all functions are unprefixed regardless of whether they are actually setters or not).

withProperty is a great convention since it makes it clear that the object is being returned again, unlike setProperty. Additionally, it makes the possibility of overlapping setters more explicit, as in withScale changing the value set by withPosition

Additionally, "with" stands out in complex expressions, since non-builders are unlikely to start a method with it.

// Consider
new ThingBuilder().x(vec.x()).y(vec.y()).build();
// vs
new ThingBuilder().withX(vec.x()).withY(vec.y()).build();

Alternatively, we could recommend prefix-less ("naive") setters in simple cases, but denounce the use of combining prefix-less setters with more complex setters (In essence, only allow prefix-less setters when all setters do nothing but set the property they are named after). This would almost certainly become a problem because of backward compatibility, though (What if Mojang adds a non-naive setter to a previously naive public Builder?).

Either way, the recommendation is still pretty ambiguous when it comes to things like addProperty or even setProperty(String name, Property value), so I additionally request discussion on the following:

Array-like properties

If withProperty were to be used for adding a property as well as setting it, it would quickly become unreadable as to if a property is being appended or replaced. I therefore propose adding an explicit section denouncing the use of property/withProperty for appending to array-like properties.

An easy alternative would be to recommend addProperty, but I can see reason for something like withAddedProperty since it keeps the same layout and (as mentioned above) makes it easier to see which methods are being invoked on/returning the builder in an expression with lots of parentheses.

Key-like properties

With some mappings I did recently, I unfortunately had to deal with a Builder Builder#property(String name, Value value); (*). This naming is not descriptive at all. It is unclear what exactly this function is supposed to do unless you read the underlying code (Thought: prefix-less property methods are supposed to be setters for changing a field, but then what would builder.property("a", 1).property("a", 2) be supposed to do?).

(*) = Actually, it was named "Factory" as well :expressionless:

I think withProperty(String name, Property value) is probably fine, it conveys no less information than setProperty, and as with above, a big plus for with is that it makes it clear we are returning the Builder instance. I'd like to hear alternatives, though.

One disadvantage of overloading with is that it may create ambiguity, although this probably isn't very significant. (Example: withText(Pair<String, String>); vs withText(String, String);)

I strongly disagree with many of your suggestions.

Factory vs Builder

Reusability
- A factory can be reused a few times
- A builder is generally single-use oriented
Mutability
- A factory usually does not mutate itself when creating an instance
- A builder mutates itself so it can better create an instance
Calling
- A factory usually accepts all required parameters at once like a constructor
- A builder gets called, keeps the parameter to itself (mutates itself) and feed to the target object's constructor later

Imo we should name classes based on the rules set above.

Builder setters

withXxx is definitely bad. In here it is usually on immutable objects such as BlockState or VillagerData, indicating the return of another object than mutating the object itself. Builders are supposed to be simple (and possibly represent keyword arguments in other programming languages) so why not builder.x(x).y(y) than setX(x).setY(y) or withX(x).withY(y), both of which are longer and usually exist on mutable objects (set) or immutable objects (with)?

Others

For array and key like properties, I will see your examples and consider as I cannot grasp it well from your descriptions.

I agree with your description of Factory and Builder, that might be good to add to CONVENTIONS.md.

I dislike the set variant for that reason as well, although with fits in my opinion, since a Builder can be seen as an immutable object that returns a new builder every time (although I can see the prefix be associated with allocation).

not that setProperty is uncommon in other code bases, but that shouldn't decide the discussion here.

I have moved this from #1940 to keep the discussion in one place.

-METHOD method_29730 criterion (Ljava/lang/String;Lnet/minecraft/class_184;)Lnet/minecraft/class_5377;
+METHOD method_29730 withAddedCriterion (Ljava/lang/String;Lnet/minecraft/class_184;)Lnet/minecraft/class_5377;

This modifies itself. So criterion is good enough.

I disagree. Consider:

new ThingBuilder().position(0,0,0).position(1,1,1).position(2,2,2).build();

We intuitively expect that this "modifies" the position property, and can conclude that the first two invocations of position are redundant, and in fact, if you've spent some time using the Builder pattern this is what you come to expect from unprefixed functions without a verb. Even if you add a verb, it is almost always set anyway, so removing the prefix in this extremely common case is acceptable. So far so good. The way worded here, "modifying" the property, is exactly the behavior I would expect from this kind of function.

However, if we follow this precedence set in the code that this pull request modifies, we always need to have the possibility in the back of our head for the rare case that the position method doesn't actually "modify" the position property, but rather "adds" a position property to Thing (Technically, in this case, it "modifies" the criteria property, but this is an implementation detail that should not be exposed through naming conventions). The easiest solution to avoid ambiguity would be to add a verb (addPosition)

In fact, several style guides recommend always using verbs (e.g. withPosition/setPosition and addPosition) for builders to avoid this exact confusion, which is a possibility I brought up, though as expected, I have already seen resistance so far, which is acceptable (If people using fabric are used to prefixless setters we should probably keep them that way).

As another example, StringBuilder uses the verb append instead of the abstract property string it is modifying:

new StringBuilder().append("hello ").append("world!").build();
// vs
new StringBuilder().string("hello ").string("world!").build();

Another solution several codebases use is to explicitly allow key-value setters, such as criterion(String name, Criterion value);.

in that case, you could write

new ThingBuilder().position(0, new Vec3f()).position(1, new Vec3f());

to pretty unambiguously add multiple positions to the Thing we are building given an index, but as I already talked about, this has several problems still.

First, we don't have control over Minecraft's code, so we can't choose which option to take for the mappings, and Mojang may decide to add an addProperty in either style at any point.

Second, this solution is still ambiguous with respect to alternative setters. I deliberately chose ThingBuilder#position(int idx, Vec3f position);, but in the case of ThingBuilder#position(0,0,0,0); it might be expected to be setting a Vector4f position instead.

In essence, this solution would still be requiring the programmer to understand what a Thing is before they can understand what ThingBuilder does, making things harder for new contributers as well as "Future Me". Additionally, the key-value case is still far less common than the simple setter case, so new contributors might not have stumbled upon a naming convention like this before and expect the position property to be unambiguously overwritten as explained above.

That is why I am proposing to only allow unprefixed setters for methods that actually modify the property, and use a different naming convention for methods that add a property (Whatever way they do it).

Actually, I personally would prefer adding a verb to every type of setter, but that is personal preference.

I'd say even though criterion looks like setting than adding a criterion, there has been convention in Gradle, a (almost required) tool chain for minecraft modding. For instance, it has methods like classpath beyonds getClasspath and setClasspath in JavaExec. The classpath does not replace existing classpath like setClasspath but adds to it, just like what the criterion does.

Perhaps a script could be written to search for builder patterns and give mapping suggestions.

FabricMC / yarn