Yinheli's Code Review Style

Analysis of Yinheli’s code review style based on 100+ comments across 50+ MRs. Use this guide for self-review before submission or to conduct reviews in a similar style.

Core Focus Areas

1. Code Simplification & Clarity (Primary Focus)

Yinheli focuses on making code cleaner and more straightforward.

Examples:

“Can use onlyTrashed() so don’t need to check trashed status”

“$user->type 已经判断了 ADVISOR，可以安全使用 Advisor::findOrFail, 可省去 $advisor === null 判断”

“用 Cache::store('redis')->add(xxx)?”

“考虑 Session::get(SessionEntity::KEY_IMPERSONATOR_EMPLOYEE) === null ?“

2. Type Annotations & Return Types

Yinheli pays attention to proper type documentation.

Examples:

“@return array<string,string|null>”

“fn 返回类型”

“返回值也比较难以和 csv_headers 去人肉对应，考虑返回一个 array<string, ?> 的结构？刚好 key 可以作为 csv_headers”

3. Method Naming & Semantics

Yinheli ensures method names accurately describe behavior.

Examples:

“这个方法表达的貌似并不是 append ，而是 build, 甚至还会调用 getAdultNameDetails 发生查询”

“CanWithTrashed -> WithTrashedSupport”

“I propose the direct reason here: we need to re-fetch the invoice since the update operation xxx.”

4. Design & Architecture Discussion

Yinheli engages in thoughtful architectural discussions, often with diagrams and detailed explanations.

Examples:

“看上去我们还需要个 session scope event，之前设计的场景容纳不了这个 case，用 model event 这个场景来装这个确实看着有点别扭。前端对此也还没有支持。我想一想。初步想法是扩充之前设计的场景，增加一种系统级别的 channel”

“I prefer to keep it here. Follow the Laravel design, Notification can be sent via multiple channels, like email, slack, database. Ref: https://laravel.com/docs/12.x/notifications#specifying-delivery-channels”

“我的意思是以前是 Model->xxxPolicy->Model->xxxPolicy->… 虽然是有额外的 N+1 问题，但是它没有形成环，而 UserIdentityAndAccessManager 作为公共的部分，要是在中间的某个环节导致整个调用链形成了收尾相连的环岂不危险 🙈“

5. Behavioral Verification

Yinheli digs into actual behavior to verify assumptions.

Examples:

“这里好像并不会发生密码验证的计算, 如果 retrieveByCredentials 返回空的 user 就直接跳过了。https://github.com/laravel/framework/blob/12.x/src/Illuminate/Auth/SessionGuard.php#L481 如果是这样，采样并不准确吧。”

“assert is disallowed 😅, ref: https://gitlab.rightcapital.io/php-libs/packages/-/blob/master/libs/phpstan-rules/phpstan-disallowed-calls-extension.neon#L33”

6. Null Safety & Edge Cases

Examples:

“感觉这样不太符合预期，我们只想 null 被过滤掉 🤔 array_filter(['a'=>null, 'b'=>1, 'c'=> '', 'd'=> 'd', 'e'=> 0]);”

“I prefer using null check instead of doc annotation. 🤔”

“Session::get 可能返回 null, 这个 rescue 倒是还是能预期是 null，但是里面实际走了异常捕获了吧，成本更高？“

7. Exception Handling

Yinheli prefers appropriate exception types that don’t pollute error tracking.

Examples:

“不期望这个被 sentry 捕获上报。业务上，卡信息更新时 1. 如果此前没有 zip，则 zip 必填 2. 可以单独只更新 zip。前端也有这个验证逻辑。我考虑后端改为 UnprocessableEntityHttpException”

8. Consistency & Conventions

Examples:

“Stay consistent with our own naming convention, we use zip, whereas Stripe use postal_code.”

“前面用的是 ->save() 这风格变了”

“Should use StripeConnect”

9. Static vs Instance Concerns

Examples:

“变成 class 级别了（共享），脱离了当前的实例，一个实例对其修改会影响别人”

“static?”

Self-Review Checklist

Before submitting for review, check these items:

Code Simplification

• Use Laravel helpers that combine multiple checks (e.g., onlyTrashed())
• Remove redundant null checks when prior conditions guarantee non-null
• Prefer built-in methods over manual implementations
• Consider if complex structures can be simplified

Type Safety

• Add proper @return annotations with array shapes
• Return types match actual behavior
• Array structures are well-documented for consumers
• Consider returning associative arrays for better readability

Naming

• Method names accurately describe what they do (not just part of it)
• Names don’t imply simpler operations than what actually happens
• Follow existing project conventions
• Use descriptive names that explain the “why”

Architecture

• Consider circular dependency risks
• Follow Laravel design patterns (e.g., Notification channels)
• Document design decisions in comments or discussions
• Think about how changes affect the broader system

Edge Cases

• Understand array_filter behavior with empty strings and zeros
• Prefer explicit null checks over relying on exception handling
• Consider the performance cost of exception-based control flow
• Verify actual behavior by reading framework source code

Exceptions

• Use appropriate exception types based on error tracking needs
• UnprocessableEntityHttpException for expected validation failures
• Don’t let expected business errors pollute Sentry

Consistency

• Follow project naming conventions over external API names
• Keep style consistent within the same file/class
• Use the same patterns as similar code elsewhere

Static Variables

• Avoid static variables that create shared state between instances
• Consider if static keyword is truly needed
• Understand the scope implications of class-level vs instance-level

Review Style Characteristics

Trait	Example
Suggests alternatives	”用 Cache::store('redis')->add(xxx)?”
Questions with curiosity	”这个为什么移除了？“
Provides code references	Links to Laravel/framework source code
Uses emojis sparingly	🤔 😅 🙈 for tone
Bilingual	Mixes Chinese and English naturally
Thoughtful architecture	Discusses design implications and trade-offs
Verifies assumptions	Reads framework source to confirm behavior
Collaborative	Engages in back-and-forth design discussions

Common Phrases

• “考虑…” / “Consider…” — Suggesting alternatives
• “可以…” / “Can use…” — Offering simplifications
• “这个为什么…” — Questioning changes
• “I prefer…” — Stating preferences with reasoning
• “感觉…” — Expressing intuition about code smell
• “ref: …” — Providing evidence with links

Patterns to Avoid

Based on Yinheli’s reviews, avoid these patterns:

1. Redundant null checks when prior conditions guarantee safety
2. Exception-based control flow for expected null cases
3. Static variables that create shared instance state
4. Method names that don’t describe actual behavior
5. External API naming over project conventions
6. Orphaned array structures without type documentation
7. Using rescue for simple null checks (performance cost)

AI Tool Usage

Yinheli actively uses AI tools in code review:

“@silly-cat review”

“/review —model=claude-3-7-sonnet —delete-old-comments”

“AI 生成了几个名字，首推 Broadcastable，但是感觉太泛了…”

Summary

Yinheli’s review style is pragmatic and verification-focused, emphasizing:

1. Code simplification — Use built-in helpers, remove redundant checks
2. Type clarity — Document array shapes and return types
3. Accurate naming — Method names should describe full behavior
4. Architecture awareness — Consider circular dependencies and design patterns
5. Behavioral verification — Read source code to confirm assumptions
6. Null safety — Prefer explicit checks over exception handling
7. Consistency — Follow project conventions, maintain style coherence
8. Collaborative discussion — Engage in thoughtful design conversations