search

Hexworks / zircon

Zircon is an extensible and user-friendly, multiplatform tile engine.
https://hexworks.org/projects/zircon/
Apache License 2.0
755 stars 138 forks source link

All classes that have builders should lock down their constructors. #382

Closed nanodeath closed 3 years ago

nanodeath commented 3 years ago

Right now classes like AppConfig and ShortcutsConfig have Builder classes that should be used exclusively for constructing instances of those classes, but this isn't guaranteed because the constructors are public, so users could actually use either. Some functionality, like that introduced in #381, assumes users will be using the Builders. Plus it'd be nice to have one Right Way to do things.

Potentially every class with a builder in our Builders package needs to be examined, but the builders themselves also need to be examined. AppConfigBuilder, for example, itself has a public constructor.

The end goal here is for there to be one way to build these things, for simplicity for the builder and to make things easier for the Zircon maintainers. Every public API class in Zircon should conform to one of the following patterns:

  1. It has a public constructor and no builder, or
  2. It has an internal constructor, a corresponding builder class (which itself has an internal constructor), and the way you construct the instance is by calling a public TheClass.newBuilder().build() method.

Phase 0

  1. Identify all classes/constructors/methods that need updating.
  2. If there's a code style guide, we should add what we're doing to it. If there isn't, we should create one.
  3. (Optional) Look into tools available for Kotlin/Java that document API changes between versions.

Phase 1

  1. Add a @Deprecated annotation to all constructors/methods that are currently public, but shouldn't be.
  2. Add KDoc to each affected class indicating the recommended way of constructing that class.

Phase 2 (next major version?)

  1. Remove @Deprecated annotations added in Phase 1.
  2. Make constructors/methods internal that should be.
adam-arold commented 3 years ago

This is a great idea. I'm guilty of not making it official (that you can only build these things in the RightWay™). I'd vote for

It has an internal constructor, a corresponding builder class (which itself has an internal constructor), and the way you construct the instance is by calling a public TheClass.newBuilder().build() method.

This leads to very clean and readable code: TheClass.newBuilder().build()

adam-arold commented 3 years ago

A note on internal: it won't hide the methods for Java consumers, for that you need to use @JvmSynthetic, but it is not recommended by the Kotlin guidelines so we might want to leave them internal.

nanodeath commented 3 years ago

I thought internal methods were obfuscated in Java?

On Thu, Apr 29, 2021 at 11:59 AM Adam Arold @.***> wrote:

A note on internal: it won't hide the methods for Java consumers, for that you need to use @JvmSynthetic, but it is not recommended by the Kotlin guidelines so we might want to leave them internal.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Hexworks/zircon/issues/382#issuecomment-829509198, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAACTAJNNXXKZA67SJDTOGDTLGUANANCNFSM43XN3UHQ .

adam-arold commented 3 years ago

Unfortunately not :( They will be visible as public in the bytecode. We can keep using internal though, it is your call.

nanodeath commented 3 years ago

Well, we'll do the best we can, plus it'll be documented. Kinda ugly but we could even mark the constructors as @Deprecated if we wanted to. I'm not a big fan of that though; even though we try to make things nice for Java users, there's only so far I'm willing to go on that.

adam-arold commented 3 years ago

Let's keep the internal then and leave the rest as-is.