code-quality

Code Quality

Quality constraints during coding and refactoring (code quality SSOT), ensuring code is maintainable, testable, and appropriately extensible.

Language

• Accept questions in both Chinese and English
• Always respond in Chinese

Trigger Conditions

• The user is writing new code / refactoring existing code
• The user needs a Code Review checklist
• The user mentions MTE principles, code quality, naming, commenting, avoiding over-engineering

Core principles: MTE

All code must follow the MTE principles：

Principle	Description	Checkpoint
Maintainability	Maintainability	Single responsibility, clear dependencies, compliant naming/commenting/logging/error handling, easy to understand
Testability	Testability	Core logic is unit-testable, dependencies are mockable
Extensibility	Extensibility	Reserve reasonable extension points, interface abstraction

Core threshold table

The sole authoritative threshold source for the whole repository. Unified predicate semantics:≤ recommended value = compliant; > recommended value and ≤ maximum value = [Suggestion]; > maximum value = [Blocker]; metrics without a recommended value (duplicate code / lines per class) have only two tiers—≤ maximum value = compliant, > maximum value = [Blocker].Mirror boundary: on the agent side, references / SKILL alwayschange the pointer, do not mirror(already migrated: dev-workflow verification-flow Phase 1, this skill's refactor-signals.md); onlyuser-side artifact templates(which will be copied into the user's project, where this skill will not be present) are allowed to inline numbers, and must be annotated with "excerpted from this table; changes must be synchronized in the same commit".

To be migrated (known agent-side mirrors of this table, not yet cleared):refactor/SKILL.md:126-132——not a pure mirror; it contains this skill's private > 2× 最大值 → P0 priority mapping, so it needs to be rewritten together and is therefore handled separately.

Metric	Recommended value	Maximum value	Refactoring direction when exceeding the maximum value
Function length	30 lines	50 lines	Extract function
Number of parameters	3	5	Parameter object
Nesting depth	2 levels	3 levels	Early return, extract function
Duplicate code	—	3 occurrences (Rule of Three)	Extract common logic (<3 occurrences: premature abstraction prohibited)
Lines per class/file	—	500 lines	Split responsibilities (god class)

Module design

Single responsibility / dependency direction'smechanical determination criteriabelong to the two rows SRP / DIP under "Design principles (code level)"; this section only gives intuitive examples and does not repeat the determination.

• Single responsibility: a class/function does only one thing (UserService business / UserRepository data access / UserValidator validation are separated; counterexample: UserManager does everything)
• Dependency direction: outer layers depend on inner layers (Interface → Service → Domain → Infrastructure); depend on interfaces, not implementations
• See references/mte-examples.md

Naming Conventions

General Principles：

1. Precision first: Reduce length by removing redundant words (do not repeat type information); truncating words is forbidden (evidence: full words locate bugs 19% faster than abbreviations; names of 10-16 characters have the lowest debugging cost)
2. Clear at the call site: The name reads like a natural phrase at the call site (clarity at the point of use)
3. Project consistency: Same concept, same name; follow the project's existing casing conventions rather than language defaults; before introducing a new term, first grep the project's existing vocabulary

Category Rules

Category	Rule
Class/Interface	Noun phrase, describing the role rather than the implementation; ending with vague suffixes such as Manager/Helper/Util/Processor/Info/Data is forbidden
Method/Function	Use a verb phrase for side effects, a noun for pure queries; use only one verb for the same action across the entire codebase (choose one of get/fetch/retrieve);get* side effects are forbidden
Variable	Length is proportional to scope: loops/closures ≤10 lines may use idiomatic single letters (i/j/k/err/ctx); function-level ≥1 full word; exported symbols by default ≥2 words and self-contained in context*
Boolean	Reads as an assertion (isEmpty/hasChildren/canEdit);is*/has* must return a boolean; negated names (isNotReady) are forbidden
Constant	Name magic numbers by meaning (MAX_RETRIES rather than FIVE)

* When the call site already has namespace context, exempt according to language conventions (e.g., Go package name http.Client, Swift type namespace).

Interface / Protocol Naming Details

In addition to the general "Class/Interface" rules (noun phrase, describe the role without exposing the implementation),without breaking the project's existing consistency, prioritize naming according to language/ecosystem conventions：

Type	Convention	Example
Capability type ("what it can do")	-able/-ible suffix	Comparable、Iterable、Closeable
Single-method interface (Go convention)	Action + -er	Reader、Formatter
Role type ("what role it is")	Noun phrase	Repository、PaymentGateway

• Forbidden: I prefix (IFoo) and other type encodings, unless it is an existing project convention (e.g., .NET)—defer to project consistency.
• Business abstraction interfaceDo not expose replaceable implementation technology (UserRepository rather than MySQLUserRepoInterface, the Impl suffix is forbidden); for external protocol/technology boundary interfaces (e.g., HttpClient、KafkaConsumer), the technology name is part of the contract and is not subject to this restriction.

Naming blacklist (a hit in newly added/modified code in this diff is [Blocker]; untouched existing code is [Suggestion])

Anti-pattern	Severity
Misleading name: name does not match reality (accountList is not a List,is* returns a non-boolean,get* has side effects)	[Blocker] (highest red line: misleading names also reduce the agent's own code analysis accuracy)
Generic name used as a complete name:data/info/temp/result/obj/item/thing/flag/val(except for local idioms)	[Blocker]
Drift of multiple names for the same concept: mixing user/account/customer for the same entity (before introducing a new term, you must first grep the project vocabulary)	[Blocker]
Type encoding (Hungarian prefix strName/m_x), unpronounceable invented abbreviation	[Blocker] (interface I prefix language/project convention exceptions see the previous section "Interface/Protocol Naming Details")
Meaningless distinction (data1/data2、ProductInfo and ProductData coexisting), single letter for large scope	[Suggestion]

Boundary: This specification only judges structural compliance (whitelist/blacklist, part of speech, consistency), and does not adjudicate specific word choices. Positive and negative examples see references/naming-examples.md。

Comment Specification

Three-way classification of information destination: Source code comments = stable facts for future readers; commit message = description of the current round of changes; traceability/task documents = process traceability. The only legitimate use of comments is to carry information that code cannot express—if it can be expressed through naming/refactoring, do not write a comment, and the latter two types of information are forbidden from being written into source code comments.

Whitelist (comments must belong to one of the following categories)

Category	Judgment
Public contract	Above exported API, explaining the calling contract/errors/boundaries/invariants, without restating the function name and types
Non-obvious why	External protocols, compatibility, performance, concurrency, security, algorithm trade-offs; after deletion the reader cannot determine "why it is written this way"
Invariants/risk constraints	Business constraints that the type system and test names cannot express, adjacent to validation or state transitions
Complex what + source	Complex algorithms/regexes/anti-idiomatic pitfall-avoidance code, explaining what pitfall would be hit if not written this way; sources are limited toExternal stable sources(paper/RFC/issue links), references to in-project process documents (design/AC/UT) belong to traceability and do not go into comments
Suppression type	eslint-disable / ts-ignore etc. must be accompanied by a specific reason
Temporary scaffolding	Limited to S2/S3 skeleton stages (TODO / test.skip/todo / pure AAA placeholders); must be cleared after S4/S6/S7

File header INPUT/OUTPUT/POS self-description is independently managed by /code-self-describe and is not constrained by this specification.

Blacklist (a hit on additions/modifications in this diff is [Blocker]; judged by semantics, keywords serve only as signals)

Category	Judgment
Changelog style	The comment describes the current round of change process rather than the current state of the code (signal words: this time/newly added/modified/fixed/as required; describing a business rule such as "new users go through the invitation code channel" is legitimate)
Speaking to the reviewer	Words such as "fixed here" "handled as required" that are oriented toward the reviewer/agent rather than future readers
Source-recording/comment-style traceability	Recording process sources such as "from UT-XX/AC-XX/postcondition/behavior contract". ⛔ Must not add @satisfies / @requirement / @verifies / @testcase annotations—numbered references pointing to external systems are pure noise to maintainers without keel context, and after keel is lost they are pointers to the void; traceability goes on the document side (reverse dependency). Existing annotations are left uncleaned, and only additions are constrained
Restating code	Merely translating the adjacent code (// 校验邮箱 + validateEmail(email)）
Commented-out code	An entire old implementation is kept commented out with no issue/migration reason
Excessive comments	More than 3 consecutive comment lines in the same function, or changed block comment lines > executable code lines, and not on the whitelist

Grading

• [Blocker]: Blacklist comments added/modified in this diff; suppression comments without justification; leftover skeleton/scaffolding comments after S4/S6/S7; comments inconsistent with the current semantics of the code (when code changes, affected comments must be updated in sync; stale comments are worse than no comments)
• [Suggestion]: Pre-existing bad comments (not touched in this diff); public API missing contract comments (escalated by the corresponding risk only when the absence has already caused security/compatibility/behavior ambiguity)

Logging Standards

The sole goal of logging isto enable rapid troubleshooting / auditing / observability of production failures—the value lies in "the right level + the right context + no leaks", not in volume; bad logs are the runtime version of bad comments (see the previous section), and are likewise graded by hits added/modified in this diff. For "where to place log points" during the design phase, see system-design log-design-guide.md; this section is thecoding discipline SSOT。

Six Hard Rules

Rule	Key Judgment Points
Purpose Threshold	Every log must serve troubleshooting/auditing/observability; prohibited: here/step1/method entry-exit/"execution completed" and other placeholder logs with no object and no result
Level Discipline	For ERROR/WARN/INFO/DEBUG·TRACE admission, see the table below; the most common misuses: treating all exceptions as ERROR, INFO flooding the screen, treating WARN as INFO
Minimal Context	Business logs contain event + result + correlation id + component + onesecurityprimary object id; failure logs additionally include error_code/exception type + cause + action taken; carry them in structured fields, and do not concatenate key context as plain text (field names follow the project vocabulary)
Exception Discipline	Exceptions are logged with a full stack trace only athandling boundaries; log-and-throw is prohibited (logging at each layer and then rethrowing = duplicating the "root cause"); swallowing an exception and only logging it is prohibited (unless the contract explicitly states it has been degraded/compensated and notes this)
Security Red Lines	Prohibited in logs: passwords, tokens, session/cookies, keys, verification codes, raw request/response bodies, raw SQL/queries, PII, payment/banking/medical data, unmasked raw third-party or LLM responses
Facts First	Write immutable facts + system actions; speculation/risk/model judgments must carry a source, rule name, or confidence (prohibited: payment service is broken, write upstream_status=504, retry_scheduled=true）

Level Admission (expansion of the "Level Discipline" rule)

Level	Admission Criteria
ERROR	The current request/task/transaction failed, is perceivable by the user or system, requires investigation, or can trigger an alert
WARN	Abnormal but already handled, and the system can continue: degradation, precursor to retry exhaustion, configuration risk
INFO	Low-frequency and valuable long-termmilestones: start/stop, configuration version, task summary, key state changes
DEBUG / TRACE	Defaultdisabled in productiondiagnostic details; failures that must be investigated must not be placed only in DEBUG

Grading

• [Blocker]: Hits added/modified in this diff on security red lines, log-and-throw duplicate stack traces / swallowing exceptions and only logging them, or writing speculation as fact
• [Suggestion]: Level misuse, insufficient context, or placeholder logs added/modified in this diff; pre-existing bad logs (not touched in this diff)

Not hardcoded (left to project conventions)

Log format (JSON/logfmt), field casing, per-business-event checklist, sampling rate, retention period,after redaction or truncationSQL/HTTP payload policy—depends on the runtime platform and regulations, hardcoding is harmful. (Note:raw SQL/request/response bodies always fall under the security red line, forbidden from logs, and are not among the things "left to the project".)

Error handling (coding level)

Only define coding-time red lines; error code system / external error contract, etc.design-levelbelong to system-design. Same origin as, and complementary to, the logging "exception discipline".

• Don't swallow exceptions: after catching, must handle, convert, or rethrow with context; empty catch / only log then continue as usual (state has drifted) → [Blocker]
• Preserve cause: when wrapping exceptions, attach the original cause/stack trace, don't lose the root cause; exception conversion only athandling boundarydone once
• Errors have semantics: use typed errors / error codes to express distinguishable failures, so callers can branch on them; don't carry errors with bare strings or booleans
• Boundary fail-fast: external input / invariant violations fail as early as possible at the boundary, don't let bad data penetrate deep into the domain
• Don't leak internals externally: errors returned to users/upstream contain no stack traces, SQL, internal paths, or implementation details (consistent with the secure coding red line and the logging security red line) → [Blocker]
• Recoverable vs unrecoverable triage: recoverable (retry/degrade/compensate) must have an explicit policy, unrecoverable fails directly, no silent fallback

Severity: swallowing exceptions / losing cause / leaking internals externally = [Blocker] (newly added/modified in this diff); the rest are [Suggestion].

Secure coding red lines

General coding-time red lines (hits in this diff's additions/modifications default to [Blocker]); deep security (threat modeling / crypto selection / compliance) is left to the project or dedicated specifications.

• Input is untrusted: all external input (user / network / file / upstream) is validated before entering the domain; SQL parameterized, output escaped per context (prevent injection / XSS)
• Authorization on the server side: permission / privilege-escalation checks must be server-side, hiding on the frontend doesn't count as a control
• Secrets and sensitive data: secrets / tokens / PII are not hardcoded, not logged, not committed to version control (see logging security red lines); transmission and storage encrypted as needed
• Least privilege: components / credentials take only the permissions they need
• Error messages don't leak: external errors don't expose internal implementation (consistent with error handling)
• Dependency security: don't introduce dependencies with known vulnerabilities, don't use deprecated algorithms (MD5/SHA1 for storing passwords, DES, etc.)

Types and data contracts

• External data is validated into domain types/shapes before entering the domain: after deserialization from network / storage / config, validate into domain types immediately, don't let unvalidated data flow into business logic (type contractdimension; injection / XSS / privilege escalation, etc.securitydimension's validation severity belongs to the secure coding red lines, not re-judged here)
• Explicit modeling of null / optional: Express "may be absent" through language mechanisms (Optional / nullable annotations / Result), not by roaming with implicit null
• Avoid stringly-typed: Use enums or types for finite states / categories, not bare strings to carry domain semantics
• Do not bypass the compiler: Prohibit broad types (any / interface{} / casts) from masking type problems, unless adapting at boundaries with justification
• Separate DTO from domain: Separate transport / persistence models from domain models, with conversion done at boundaries

Grading: stringly-typed, abusing broad types to bypass the compiler, missing type/shape validation = [Suggestion] (security dimension's missing input validation grading falls under the security coding red line).

State and side effects

• Prefer pure functions for core logic: Business computation has no side effects and is replayable (for the testability perspective see "Testability Design"; not repeated here)
• Concentrate side effects at boundaries: IO / global state / time / randomness are concentrated in the boundary layer, keeping the domain layer pure
• Shared mutable state must have an owner: Shared mutable state across threads / coroutines must have clear ownership or synchronization; if it can be unshared, unshare it
• Async must be cleaned up: Async / background tasks must consider cancellation, timeout, idempotency, and resource release, leaving no dangling tasks

Design principles (code level)

Themechanically determinable code-level/diff-level characteristicsof the six principles of SOLID + Law of Demeter. Design-level determination (heuristic thresholds / three-state conclusion table / ADR linkage) is authoritatively assigned to system-design solid-principles-guide.md, and this section does not replace it.

Principle	Code-level determination characteristic
SRP Single Responsibility	Class/function responsibility description contains multiple-responsibility conjunctions such as "and/also"
OCP Open-Closed	Adding a new type requires modifying existing switch/if-else branches to extend
LSP Liskov Substitution	Subclass/implementation throws exceptions outside the parent class (interface) contract, or strengthens preconditions/weakens postconditions
LoD Law of Demeter	Chained cross-layer calls a.getB().getC().getD()(crossing >2 layers; fluent API/builder excluded)
ISP Interface Segregation	Single interface has >7 methods, or consumer usage ratio <50% (fat interface)
DIP Dependency Inversion	Cross-module dependencies reference concrete implementation class names rather than interfaces

Grading: DIP violations are [Blocker] (aligned with design-level hard constraints); the other five are [Suggestion] (heuristic; violations must be justified).

Testability design

Three elements of testable code (for detailed test-writing specifications see /testing-guide）：

1. Dependencies injectable — external dependencies are passed in via parameters/constructors, not hardcoded
2. Prefer pure functions — business logic has no side effects, same input yields same output
3. Boundary separation — business logic is separated from IO

For positive and negative examples see references/mte-examples.md。

Avoid over-design

• YAGNI: Do not add configuration/extension points for hypothetical requirements
• Rule of Three: Create an interface only when there are 3 or more implementations (exception: Across-module boundaries / external dependencies / testability boundaries; to decouple, interfaces may be created first); duplicates ≤3 are compliant (the 3rd instance is suggested for extraction, non-blocking; <3 instances must not be abstracted prematurely), >3 instances not extracted = [Blocker]; only create a base class when there are more than 3 similar scenarios
• Prefer simple solutions: Do not apply complex patterns to simple scenarios
• Optional enhancement · Specialized complexity review: If the environment provides review capability specifically for over-engineering (such as /ponytail-review reviewing diffs,/ponytail-audit reviewing the entire repo), an additional round may be appended after the self-check in this section—it only looks for "what should be deleted," complementing this skill's correctness + threshold review.If that capability is unavailable, stop at this section's checklist, ⛔ do not skip or reduce the minimum review checklist because of its absence; its conclusions must be normalized according to this skill's feedback levels before being recorded in the review report, ⛔ do not establish a separate set of levels
• For positive and negative examples, see references/mte-examples.md

Design Pattern Usage

• Use only when necessary: Solve real problems, do not use for showing off
• Explain the rationale: Why this pattern was chosen
• Stay consistent: Use the same pattern for the same problem

Pattern selection is a design-time decision; the authoritative scenario→pattern mapping table is in system-design design-patterns.md(consumed by the 02-system-design §6 Design Patterns chapter); this file does not mirror it.

Refactoring Guidance

Safety constraints (non-negotiable):

• Test coverage must exist before refactoring(if there are no tests, add tests first)
• Run tests after each refactoring step, advance in small steps
• Do not add new features during refactoring, do not refactor while adding features
• A single refactoring should address only one type of problem

For Code Smells trigger signals, risk assessment, and detailed process, see references/refactor-signals.md; for the systematic refactoring workflow (scope analysis + test gate), use /refactor。

Code Review

Feedback Levels

Repo-wide feedback levelsmeta-semantics(the meanings and handling requirements of Blocker/Suggestion/Question/Nice) use this table as authoritative; each review scenario (verification-flow / ui-quality-checklist / adversarial-review, etc.) only defines domain-specific judgment items and does not redefine level meanings.

Level	Mark	Meaning	Requirement
Blocking	[Blocker]	Must be fixed before merging	Security issues (including secure coding red lines), logic errors, exceeding the maximum values in the core threshold table, naming/comment/log blacklist hits, swallowing exceptions/losing causes/leaking internal details externally, DIP violations
Suggestion	[Suggestion]	Recommended change but non-blocking	Exceeding recommended values, code style, minor optimizations
Question	[Question]	Requires author explanation	Design decisions that are not understood
Praise	[Nice]	Something done well	Encourage good practices

Minimum Review Checklist

• All core threshold tables are compliant (function/parameters/nesting/duplication/single class)
• No naming blacklist hits (misleading names/generic names/same-concept drift/type encoding)
• No comment blacklist hits (changelog-style/speaking to reviewers/source-recording style/restating code)
• Design principles (code level): six items with no hits
• Dependencies injectable, business logic separated from IO
• No over-engineering (YAGNI / Rule of Three)
• Error handling: do not swallow exceptions, preserve cause, do not leak internals externally (see the error handling section for details)
• Types and data contracts: validate external input before entering the domain, no stringly-typed / broad types bypassing the compiler
• Logging: correct levels, include correlation id + minimal context, no log-and-throw (hitting a security red line is a Blocker, see the logging specification for details)
• Secure coding red lines: input validation and parameterization, authorization on the server side, secrets/PII not hardcoded and not output (injection/XSS/privilege escalation = Blocker)

Feedback must be graded and justified; security issues must be [Blocker]. For the complete review dimensions and output format, see references/review-rubric.md。

References

File	Content
references/mte-examples.md	Positive and negative example code for modules/functions/testability/YAGNI
references/naming-examples.md	Positive and negative naming examples, casing conventions, namespace exceptions
references/refactor-signals.md	Code Smells signals, refactoring process, risk assessment
references/review-rubric.md	Detailed Review checklist and output format