Yan's Code Review Style

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

Core Focus Areas

1. Integration Domain Expertise (Primary Focus)

Yan deeply understands integration patterns and consistently reviews integration-related code with domain expertise.

Examples:

“why it’s under API based? I thought it would apply to file-based as well. Considering we shall add scenarios for all integrations in the future, do we need to define another enum?”

“For Integrations that have multiple codes, for example Pershing, how should we handle such references?”

“why not re-use the trashed integration if the rep code exists being soft deleted?“

2. Naming Clarity & Grammar

Yan pays close attention to naming accuracy and English grammar.

Examples:

“What does met mean here? Eligible is much better than meet, as met has many meanings in different situations.”

“grammar error: isContains”

“Inactive is an adjective, optional: deactivate”

“less than” (spelling correction)

“conditions” (plural correction)

“is_deleted” / “复数” (number agreement)

3. Logging & Debugging Context

Yan emphasizes proper logging for debugging purposes.

Examples:

“Adding a log is more helpful in determining if the command has finished.”

“Perhaps recording the unsatisfied condition to the log is helpful for debugging.”

“it’s better to add some comment”

“it’s better if integration id is recorded.”

“建议打个日志记录一下”

4. Code Simplification

Examples:

“No need to use such a complicated query. you can use where(COLUMN_INTEGRION_ID, {ID}) directly.”

“The counter is not needed if only updating one account.”

“No need to use an extra variable here.”

“Alternative: $record[‘cusip’] = $record[‘cusip’] === ” ? null : $record[‘cusip’];“

5. Fixture & Test Data Quality

Yan carefully reviews test fixtures and ensures they represent real scenarios.

Examples:

“Should we update the fixture file?”

“Not a good fixture data to use ID 5(A360) to test sso binding”

“This type of test data is not relative to the code changes above.”

“why don’t you use assertJsonApiCall()?“

6. Branch & Commit Conventions

Examples:

“Adding a brief explanation in the commit message is more helpful in understanding the change.”

“branch name can be more specific”

“你这个属于 hotfix，分支名字错了。基于的 base branch 也错了。”

“branch name 没有 ticket id”

7. Security & Configuration

Examples:

“任何直接把密钥提交到 git commit 里面的行为都是不符合安全规范的”

“DB UPDATE privilege is not granted for both 2 tables.”

“staging 也需要排除掉吧？“

8. API Design & RESTful Consistency

Examples:

“assignEntityToHousehold is more like a business description, but storeHouseholdMapping is not.”

“Is Front-end team aware of this change?”

“Did the Front-end team make the corresponding changes to include all these required scopes?”

Self-Review Checklist

Before submitting for review, check these items:

Naming & Grammar

• Method/variable names are grammatically correct
• Use proper singular/plural forms
• Adjectives vs verbs used correctly (inactive vs deactivate)
• Names describe business purpose, not implementation

Logging & Debugging

• Add logs at key decision points
• Include relevant context (integration_id, reference, etc.)
• Log both success and failure paths
• Command completion is logged

Integration Patterns

• Consider both API-based and file-based integrations
• Handle soft-deleted integrations appropriately
• Reuse existing integration records when possible
• Consider multi-code integrations (e.g., Pershing)

Test Quality

• Fixture data represents real scenarios
• Test data IDs are meaningful, not arbitrary
• Use proper assertion helpers (assertJsonApiCall)
• Explain non-obvious test setups with comments

Branch & Commit

• Branch name includes ticket ID
• Branch type matches change type (bugfix/, hotfix/, feature/)
• Base branch is correct (master for hotfix)
• Commit message explains “why”, not just “what”

Configuration & Security

• No secrets in git commits
• Database privileges are granted
• Environment-specific configs are correct
• Staging/UAT configs are considered

Code Quality

• Avoid unnecessary variables
• Use simple queries when possible
• Remove redundant counters
• Check for simpler alternatives

Cross-Team Coordination

• Frontend team is aware of API changes
• Admin Center API is updated if needed
• Notion schedule is updated for jobs

Review Style Characteristics

Trait	Example
Grammar-conscious	”less than”, “conditions”
Domain expert	Deep integration knowledge
Bilingual	Switches between Chinese and English naturally
Detail-oriented	Catches small issues like pluralization
Helpful suggestions	Provides alternatives, not just criticism
Cross-team awareness	”Is Front-end team aware of this change?”
Security-minded	”任何直接把密钥提交到 git commit 里面的行为都是不符合安全规范的”

Common Phrases

• “可以/建议…” — Gentle suggestions
• “为什么…” / “Why…” — Questioning design decisions
• “这个需要…” — Identifying missing elements
• “is more helpful” — Suggesting improvements
• “final” — Marking classes as final
• “复数” / “singular/plural” — Grammar corrections

Integration-Specific Patterns

Yan’s reviews often reference specific vendor patterns:

• Yodlee: Provider accounts, soft deletion handling
• LPL: Rep codes, audit files, certificates
• Orion/Panoramix: Response structure handling
• Schwab: Account number matching
• SAML SSO: Certificate management, inbound/outbound configs

Summary

Yan’s review style is domain-expert and detail-oriented, focusing on:

1. Integration expertise — Deep knowledge of vendor-specific patterns
2. Grammar precision — Correct English usage in code and messages
3. Logging completeness — Ensure debugging is possible
4. Test quality — Fixtures should represent real scenarios
5. Cross-team coordination — Consider frontend and admin API impacts
6. Security awareness — No secrets in commits, proper privileges
7. Helpful alternatives — Provide simpler solutions when possible