Leo's Code Review Style

Analysis of Leo’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. API Design & Developer Experience (Primary Focus)

Leo cares deeply about whether APIs are intuitive and hard to misuse.

Examples:

“This method is not developer-friendly, they have to check: 1. If there is an exception thrown 2. If the return value is null 3. If the response is successful 4. If it’s high risk scope”

“It’s a bad design here, $related_model is an empty model, but other developers will not be able to aware of this according to its naming.”

“Have you considered using getter here? $qualified_foreign_key is derived from $foreign_key and it’s error-prone if both of them are passed from caller.”

“I prefer to make this method abstract, so for every model the developer needs to consider if it can be modified by client”

2. Type Precision (class-string Focus)

Leo prefers precise types, especially class-string over model objects when the object’s data isn’t needed.

Examples:

“Unnecessary to be a model object, class-string is enough. Please check all methods with $model parameters.”

“We should add a class-string<> doc to restrict the type of parameter”

“We only call $owner_model::class in this method, why not change it to class-string?”

“If getOwnerModelAccessibleIds accepts a class-string instead of Model, I don’t think we need to query database, we can infer it from code”

3. Performance Awareness

Leo consistently identifies performance issues.

Examples:

“This is not a core function, we can make it async to reduce login api latency.”

“A potential performance issue here: when the getOwnerModelId returns null, rememberForever will call getOwnerModelId again even if it was already called previously”

“I don’t think 15 minutes is a reasonable cache ttl, we should only send an email per day.”

“It’s not a good practice to initialize all drivers, check Laravel’s built-in manager classes.”

“A potential N+1 issue”

4. Comment & Documentation Accuracy

Leo verifies that comments accurately describe code behavior.

Examples:

“I don’t think this comment is accurate, before will be called before any method inside the policy, not only write methods.”

“This is no longer true, the exclusion now happens inside SavingStrategy model using GlobalScope instead of policy”

“There is an outdated comment still refers to resolveReadableIdsForDependent”

“The comment needs update”

5. Future-Proofing Design

Leo considers how code might be extended or misused.

Examples:

“We cannot tell that we won’t add read method to policy in the future, it’s a bad design to call violatesWriteRestrictionOnClientPermission in before.”

“This method is only called in UnifiedWrite. It’s causing incorrect usage if we put this in DependentPolicy”

“This is a bad design from the first day, we should change it to a method and allow subclass to override it”

6. Code Organization & Architecture

Leo has strong opinions on where code should live.

Examples:

“I don’t think this function is tied to DistributedLock, we can move it to app/Support/helpers.php”

“I prefer to move this method into is_unique_job_ongoing, we should only add new code to Extensions when necessary.”

“This controller doesn’t use any abilities from BulkModelController, it doesn’t need to inherit BulkModelController”

“I don’t think CalcEngine is suitable here, CalcEngine won’t call these APIs.”

7. Naming Precision

Leo catches ambiguous or misleading names.

Examples:

“The name is inaccurate, there are several types of impersonation, employee impersonation is one of them”

“The name is confusing”

“The method name should be getPaginationSortStrategiesByField according to the return type”

“I think webauthn is better than passkey”

8. Exception Handling

Leo prefers exceptions over silent failures or assertions.

Examples:

“throw Exception instead of assert”

“I prefer throw exception in test”

“Should throw exception if owner is not a Person”

“I prefer reporting to Sentry, nobody cares warning logs.”

Self-Review Checklist

Before submitting for review, check these items:

API Design

• Methods have a single, clear way to check success/failure
• Parameter names accurately describe what’s expected
• Derived values are computed internally, not passed by callers
• Error-prone parameter combinations are avoided
• Abstract methods force developers to make conscious decisions

Type Safety

• Use class-string<T> when you only need the class name
• Add @param class-string<> annotations
• Avoid passing model objects just to call ::class on them
• Define phpstan-type for reusable complex types
• Use list instead of array for sequential arrays

Performance

• Non-critical operations are async where possible
• Cache TTLs match business requirements
• No repeated expensive operations (cache results)
• Lazy initialization for drivers/services
• Check for N+1 query issues

Comments & Documentation

• Comments accurately describe current behavior
• Outdated comments are updated
• Complex code has explanatory comments
• Add comments for non-obvious design decisions

Design Considerations

• Abstract methods force consideration of edge cases
• Code placement considers all subclasses
• Inheritance is justified (not just for code reuse)
• Extensions are only added when truly necessary

Code Organization

• Helper functions go in appropriate locations
• Controllers inherit only what they need
• Related code is grouped together
• Namespace reflects actual purpose

Naming

• Names are accurate and unambiguous
• Method names match return types
• Variable names describe content, not implementation
• Branch names follow conventions (bugfix/, hotfix/)

Error Handling

• Prefer exceptions over silent failures
• Report to Sentry, not just logs
• Tests use assertions, not silent passes
• Error messages provide useful context

Review Style Characteristics

Trait	Example
Developer empathy	”developers will not be able to aware of this according to its naming”
Precise type preferences	”class-string is enough”
Questions purpose	”What’s the purpose of this change?”
Considers future extensions	”We cannot tell that we won’t add read method in the future”
References Laravel conventions	”check Laravel’s built-in manager classes”
English communication	All comments in English
Links to discussions	References chat.rightcapital.ai for context
Architectural thinking	”we should only add new code to Extensions when necessary”

Common Phrases

• “I don’t think…” — Expressing disagreement
• “I prefer…” — Stating preference
• “What’s the purpose of…” — Questioning design
• “It’s a bad design…” — Identifying anti-patterns
• “We should…” — Prescriptive guidance
• “It’s error-prone…” — Identifying risks
• “This is not a good practice…” — Citing best practices

Patterns to Avoid

Based on Leo’s reviews, avoid these patterns:

1. Passing model objects just to get class name
2. Multiple ways to check method success/failure
3. Silent failures instead of exceptions
4. Warning logs instead of Sentry reports
5. Inheriting controllers without using their capabilities
6. Initializing all drivers upfront
7. Outdated comments that don’t match code
8. Assertions in production code instead of exceptions

AI Integration

Leo actively uses AI tools for code review:

“I would like to start building knowledge for AI coding agents. Can you put a readme.md file in app/Models/Policies?”

“Ask AI to polish this message”

“For commit message: https://chat.rightcapital.ai/chat/…”

Summary

Leo’s review style is pragmatic and architecture-focused, emphasizing:

1. Developer experience — APIs should be intuitive and hard to misuse
2. Type precision — Use the most specific type (especially class-string)
3. Performance — Async operations, proper caching, lazy initialization
4. Accurate documentation — Comments must match current behavior
5. Future-proofing — Consider how code will be extended
6. Proper placement — Code lives where it belongs architecturally
7. Exception over silence — Report errors, don’t hide them