yuqi1129 commented on code in PR #11339:
URL: https://github.com/apache/gravitino/pull/11339#discussion_r3338515879
##########
AGENTS.md:
##########
@@ -42,10 +42,54 @@
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.
+- **Reuse existing types and patterns**: Before adding a new request DTO, PO,
or helper, check whether an existing one (for example, an `XxxAssociateRequest`
or `XxxPO`) already fits, and reuse it. Validate request DTOs at the request
boundary (reject nulls, overlapping add/remove sets, etc.).
+- **Exceptions**: Throw the standard, semantically correct exception type for
the situation (for example, the project's unauthorized/forbidden exception for
auth failures). Do not add a `catch (RuntimeException e) { throw e; }` that
adds no behavior, and collapse identical sibling `catch` blocks. When
refactoring, preserve identifying context (such as the table identifier) in
error messages and logs.
+- **Prefer standard-library and Commons idioms**: Use
`StringUtils.isBlank`/`isNotBlank` instead of manual null/empty string checks,
a `Set` (not a `List`) for repeated membership lookups, and
`Base64.getEncoder().encodeToString(...)` rather than manual byte/string
round-trips. Do not use deprecated APIs (for example,
`RandomStringUtils.randomAlphabetic`); keep a
`@SuppressWarnings("deprecation")` only when it is required for compatibility,
with a comment explaining why.
+- **Component placement & lifecycle**: Put a class in the module that owns its
concern. A sub-component of another component (for example, a side module of
the entity store) should be created, started, and closed by its owner —
mirroring sibling components such as `RelationalGarbageCollector` — rather than
being exposed and wired as a global `GravitinoEnv` component.
+- **Prefer capability interfaces over concrete casts**: Detect optional
behavior through a `SupportsXxx` capability interface plus `instanceof`, not by
casting to a concrete implementation class.
+- **Argument validation**: Use `Preconditions.checkArgument(condition,
message)` for argument/state validation; do not use `checkNotNull`. Only
null-check values that can actually be null — do not add defensive null checks
for values guaranteed non-null by their source (for example, rows returned by a
MyBatis mapper).
Review Comment:
We always need to disable `Preconditions.checkNotNull` as it will throw NPE.
--
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]
