yuqi1129 commented on code in PR #11339: URL: https://github.com/apache/gravitino/pull/11339#discussion_r3339072174
########## AGENTS.md: ########## @@ -42,10 +43,61 @@ 4. Constructors. 5. Methods (Group by visibility, putting `private` methods at the end). +## Code Design & Structure +- **Method granularity**: Each method must earn its existence. Avoid small, fragmented single-use helpers — keep cohesive logic together. At the same time, fold genuinely duplicated logic into one shared helper instead of repeating it. Extract when it is reused or names a genuinely non-obvious step; inline trivial one-liners. +- **No dead parameters or methods**: Remove a parameter that is always the same constant (for example, a flag that is always `true`) and a method that does nothing meaningful. Keep related declarations and their setup together (for example, a thread-pool field and the loop that submits tasks to it). +- **Field assignment**: Qualify field assignments with `this.` (for example, `this.workers = ...`) in constructors and setters. +- **Test-only code must not live in production classes**: Never add a production method or field that exists only to serve a test. + - Prefer keeping test-only logic in the test module (test helpers, fixtures, or the test class itself). + - If a production member genuinely must be reachable from a test, annotate it with `@VisibleForTesting` and keep the narrowest visibility possible (package-private, not `public`/`protected`). Never widen visibility solely for a test. + - Do not reach into non-public members from tests via reflection (`setAccessible(true)`). Add a `@VisibleForTesting` constructor/factory instead. +- **Narrowest visibility**: Give every class member the smallest visibility that works (`private` first, then package-private). Do not make something `public`/`protected` unless an external caller needs it. +- **Dependency injection, not singletons**: Avoid the singleton pattern (including lazy-holder/double-checked instances) — it is hard to test. Construct collaborators once in the owning class (for example, the plugin/bootstrap class) and inject them; do not `new` a manager/service inline in the middle of logic, and never create two instances of a component that is meant to be shared. +- **Naming follows existing conventions**: Match the naming patterns already used by sibling classes and the surrounding package, and make the name reflect the class's actual role. Established suffixes include `XxxManager`, `XxxDispatcher`, `XxxService`, `XxxOperations`, `XxxListener`, `XxxPoller`, capability interfaces `SupportsXxx`, and tests `TestXxx`. Capitalize acronyms consistently with existing code (`RESTUtils`, not `RestUtils`). Name boolean fields without an `is` prefix (`basicAuthEnabled`, not `isBasicAuthEnabled`). Do not invent a new convention when a matching one already exists. Review Comment: Fixed in a69361b724. Reworded the boolean naming guidance to follow the surrounding module's established convention and keep field names consistent with getter semantics, rather than forbidding the `is` prefix. Also added related rules for module/package responsibility, behavior-revealing names, and avoiding speculative abstractions. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
