Tingsong's Code Review Style

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

Core Focus Areas

1. Class Design & Responsibility Boundaries (Primary Focus)

Tingsong heavily scrutinizes whether class names accurately reflect their behavior and responsibilities.

Questions to ask yourself:

• Does the class name match what it actually does?
• Are you following established patterns correctly? (e.g., Laravel Manager pattern)
• Is this class trying to do too much?

Examples:

“这个 Job 的名字和它实际做的事情不相符”

“CaptchaManager 不提供 captcha driver 反而直接负责 verifyFromRequest？” (Comparing to FilesystemManager, DatabaseManager, CacheManager patterns)

“Captcha、CaptchaManager、Driver、Vendor、Config、CaptchaResponse 等概念不清晰”

“为什么我们的 Integration 类上没有 getIntegrator() 方法？“

2. Data Class Design

Principles:

• Data classes should be immutable
• Avoid nullable field explosion when supporting multiple vendors
• Use interfaces for unified behavior across different implementations

Examples:

“Bad design，数据类应该设计成 immutable 的”

“Vendor 一多，这个类就会充满 nullable 字段。显然不同的 Captcha vendor 提供的 response 不同，强行放入一个类里不合理。假如你希望这个类能提供统一的判断能力，比如 isHighRisk，你应该定义一个 interface”

“In fact, I think this class should accept an Eloquent relation object and most of these should be exposed via getter”

3. Honest Extensibility

If you’re not genuinely supporting extension, don’t pretend to.

Examples:

“如果我是一个自定义 Driver 的开发者，按照你的 DriverInterface 要求实现了 getPublicConfig，实现之后却根本无法正常工作，就因为这里不暴露自定义 Driver 的 PublicConfig。我只能认为你在耍我。”

“我认为你在 rename 之后就不要想着这是个 Manager 了，反正你也没独立出去做成一个 Laravel service，我们一两年内也不大可能引入第三家 Vendor，直接去掉扩展能力算了。“

4. Code Formatting & Organization

Tingsong cares about code layout, line length, and visual organization.

Examples:

“太长，请拆行”

“这个 verify 到下面使用 result 之间隔了 30 行代码，为什么不能写到一起？”

“换人了，上面留一个空行”

“冒号对齐”

“拆成多行数组吧，挤在同一行太长了。“

5. Commit Message & Documentation Quality

Examples:

“Commit message 不应在句子中间折行。”

“Commit message 可以调整一下，把避免重复存储这一变更目的写到标题行里。”

“TODO 得加下次检查的日期”

“需要注释用途”

“这个注释和下面的代码逻辑不一致，需要检查 if 的实际意图”

6. Testing Quality

Tingsong emphasizes proper testing patterns.

Examples:

“不应该在一条 test case 中 foreach，应该使用 data provider。”

“在 each callback 中的 assert 不一定能执行到，如果前面的查询没有结果，each 就不会触发 callback 运行”

“审慎评估每一项测试基线的变更是否符合预期是基线测试的基本要求。你的改动时间成本很低，但对基线进行检查的时间成本很高。”

“为了测试 sync 时会删除 inactive 的 account，向 database seed 添加一个账号而引入巨大的 baseline 变化是否值得？“

7. PHP Modern Features

Tingsong strongly advocates for modern PHP syntax.

Examples:

“switch-case 应该转为 match，这样 enum 选项增减时不会漏掉”

“应该用 RecaptchaErrorCode::from($error_code) 而非 switch-case 判断 enum backed value”

“\Closure 需要定义参数表和返回值类型”

“suggestion\nreturn Arr::first($portfolios, self::isActivePortfolio(...));\n\nFirst class callable”

“catch 的变量不使用可以不写”

8. Error Handling & Edge Cases

Examples:

“应该继承 \RuntimeException”

“InvalidArgumentException 的消息不会转化成 http response，用户将无法从 UI 上获得提示信息”

“批量删除不会触发 model event。虽然 AdvisorNetwork 目前暂时没有 model event，但可能埋坑。”

“如果是 name 的值是空字符串怎么办？”

“为什么乱传 cursor 要自动改用 1 而非报错？“

Self-Review Checklist

Before submitting for review, check these items:

Naming & Semantics

• Class name accurately describes its responsibility
• Method names reflect what they return (e.g., method returning list<T> should have plural name)
• Variable names are not inverted (e.g., use is_disabled if always used with !)
• Exception class names match their value types (e.g., “Code” implies int, not string)
• Messages have proper grammar (subject, verb, object)
• Don’t use “test” as placeholder values

Code Organization

• Related code is grouped together (no 30-line gaps between declaration and usage)
• Transform/conversion logic lives in the appropriate class
• No unused imports or methods
• Line length is reasonable, split long lines
• Proper alignment (colons, array keys)
• Blank lines separate logical sections

PHP Best Practices

• Use match instead of switch-case for exhaustive enum handling
• Use EnumClass::from($value) instead of switch-case on backed values
• Use first-class callable syntax ($this->method(...)) instead of closures
• Closures have defined parameter and return types
• Custom exceptions extend \RuntimeException
• Use Laravel Context instead of passing $log_context through call chains
• Singletons are only used when genuinely necessary
• Don’t use signature for commands, use name and getOptions()
• Use Date::now() instead of other date methods
• Avoid importing functions from “Safe” package

Design Patterns

• Manager classes provide drivers, not direct functionality
• Responsibility boundaries match class names
• Exception conversion happens at the right layer (usually the caller, not the callee)
• Default values make sense (e.g., empty string vs null for IP addresses)
• If a trait has only static methods, it shouldn’t be a trait

Visibility

• Properties and methods use minimum necessary visibility
• No unexplained public on internal implementation details

Testing

• Use data providers instead of foreach in test cases
• Assertions in callbacks will actually execute
• Baseline changes are justified and documented
• Test coverage matches actual behavior, not just happy path

Commit Messages & Documentation

• Commit message title describes the purpose, not just the change
• No line breaks in the middle of sentences
• TODOs include a follow-up date
• Comments explain “why”, not “what”
• Comments are accurate and up-to-date

Review Style Characteristics

Trait	Example
Direct and sharp	”callback 是个烂名字”
Always asks why	”why public?”, “为什么一定是 recaptcha_result？“
Points out contradictions	”名字里带 Code，但值类型不是 int 而是 string”
Provides specific alternatives	”switch-case 应该转为 match”
Uses code suggestions	Frequently uses GitLab’s suggestion blocks
Proactive investigation	Searches Sentry for related issues, expands scope of fixes
References AI for explanations	”@silly-cat 为什么在对象方法中使用 static 变量会有高风险？“
Cares about code proximity	”这个 verify 到下面使用 result 之间隔了 30 行代码”
Considers maintainability	”我们的修改依赖上游的特定版本，应该修改 composer.json 而非仅更新 composer.lock”

Common Phrases

• “为什么…” / “何不…” — Questioning design decisions
• “这个方法没用，应该删除” — Identifying dead code
• “应该…” — Prescriptive corrections
• “不要用…” — Prohibitions against bad patterns
• “请拆行” / “太长” — Formatting concerns
• “可以直接…” — Suggesting simplifications

Summary

Tingsong’s review style is design-oriented and meticulous, focusing on:

1. Naming-behavior consistency — Names must accurately reflect behavior
2. Clear responsibility boundaries — Reject classes with ambiguous responsibilities
3. Honest extensibility — If you won’t truly support extension, don’t pretend to
4. Code formatting — Proper line length, alignment, and visual organization
5. Modern PHP — Embrace match expressions, first-class callables, strict typing
6. Testing rigor — Data providers, executable assertions, justified baselines
7. Direct communication — Problems are stated plainly without hedging