====== Engineering Knowledge Learned from PR Reviews ======

===== Overview =====
This document summarizes practical engineering knowledge learned from real PR reviews.
The focus is how reviewers think, why comments matter, and how to apply the lessons in future development.

Goal:

Write code that passes review smoothly

Design systems that scale and are maintainable

Think like a senior engineer, not just an implementer

===== 1. Clean Architecture: Layer Responsibility =====

==== Knowledge ====
Each layer has a single responsibility.
Business logic must not depend on delivery mechanisms (HTTP, gRPC, protobuf).

==== ❌ Bad Example ====
Usecase handling HTTP concerns:
<code go>
if err != nil {
return makeErrorResponse(http.StatusBadRequest, "invalid request")
}
</code>

==== ✅ Good Example ====
Usecase returns domain error only:
<code go>
return errors.WithStack(ErrInvalidRequest)
</code>

Handler converts error to HTTP response:
<code go>
if err != nil {
return http.StatusBadRequest, err.Error()
}
</code>

==== Why Reviewers Care ====

Usecase should work for HTTP, gRPC, batch jobs

Easier testing and reuse

Clear separation of concerns

==== How to Apply ====
Ask before coding:

“If this was no longer HTTP, would this code still belong here?”

===== 2. Do Not Leak Protobuf / DTO into Business Logic =====

==== Knowledge ====
Transport formats change frequently.
Domain and usecase must remain stable.

==== ❌ Bad Example ====
<code go>
func Execute(req *pb.GetCouponRequest) {
if req.Mode == pb.Mode_LIST {
...
}
}
</code>

==== ✅ Good Example ====
Domain model:
<code go>
type CouponQuery struct {
Mode CouponMode
}
</code>

Usecase:
<code go>
func Execute(q CouponQuery) ([]Coupon, error) {
...
}
</code>

Handler mapping:
<code go>
q := CouponQuery{
Mode: mapProtoMode(req.Mode),
}
</code>

==== Why Reviewers Care ====

Protobuf changes should not break business logic

Domain models should be framework-agnostic

==== How to Apply ====
Rule of thumb:

If pb. appears in usecase → wrong layer

===== 3. Error Handling Is About Observability =====

==== Knowledge ====
Errors are signals, not just messages.

==== ❌ Bad Example ====
<code go>
return fmt.Errorf("invalid request")
</code>

==== ✅ Good Example ====
<code go>
var ErrInvalidRequest = errors.New("invalid request")

return errors.WithStack(ErrInvalidRequest)
</code>

==== Why Reviewers Care ====

Stack traces in logs

Easier root-cause analysis

Error comparison using errors.Is

==== How to Apply ====
Always consider:

Can this error be debugged in production?

===== 4. Validation Must Stop at the Boundary =====

==== Knowledge ====
Invalid input should never reach usecase.

==== ❌ Bad Example ====
<code go>
if req.Mode != "list" && req.Mode != "history" {
return ErrInvalidRequest
}
</code>

==== ✅ Good Example ====
Declarative validation:
<code go>
type Request struct {
Mode string validate:"oneof=list history"
}
</code>

==== Why Reviewers Care ====

Cleaner business logic

Validation rules are readable and centralized

==== How to Apply ====
Perform validation in:

Handler

Validator layer

Value Object constructors

===== 5. Domain Models Should Represent Meaning =====

==== Knowledge ====
Domain models represent concepts, not API fields.

==== ❌ Bad Example ====
<code go>
CouponID
CouponCode
CouponName
</code>

==== ✅ Good Example ====
<code go>
ID
Code
Name
</code>

==== Why Reviewers Care ====

Less noise

Easier reasoning

Domain-first design

==== How to Apply ====
Ask:

“Would this name make sense without protobuf?”

===== 6. Pointer vs Value Must Be Intentional =====

==== Knowledge ====
Pointers imply mutability and shared state.

==== ❌ Bad Example ====
<code go>
[]*Coupon
</code>

Used everywhere without reason.

==== ✅ Good Example ====
<code go>
[]Coupon
</code>

Use pointers only when:

Object is large

Mutation is required

Identity matters

==== Why Reviewers Care ====

Avoid side effects

Clear API intent

==== How to Apply ====
Be able to explain:

“Why is this a pointer?”

===== 7. External Systems Are Hostile =====

==== Knowledge ====
Never trust external APIs.

==== ❌ Bad Example ====
<code go>
resp, _ := client.Do(req)
return resp.Body.Data
</code>

==== ✅ Good Example ====
<code go>
if resp.StatusCode != http.StatusOK {
return ErrExternalService
}
if resp.Body == nil {
return ErrInvalidResponse
}
</code>

==== Why Reviewers Care ====

External services fail

Partial responses happen

==== How to Apply ====
Always validate:

Status code

Required fields

Nil / empty values

===== 8. Backward Compatibility Over Cleanliness =====

==== Knowledge ====
APIs are long-term contracts.

==== Example ====
Proto field numbers are not sequential, but not changed to avoid breaking clients.

==== Why Reviewers Care ====

Breaking changes affect other teams

Stability > aesthetics

==== How to Apply ====
Before changing:

Schema

API

Contract
Ask:

“Who will this break?”

===== 9. Refactoring During Review Is a Positive Signal =====

==== Knowledge ====
Responding to reviews with refactoring shows maturity.

==== Bad Reaction ====

“Out of scope”

“Existing code does this”

==== Good Reaction ====

Refactor

Explain trade-offs

Improve architecture

==== Why Reviewers Care ====

Ownership mindset

Long-term thinking

===== 10. Tests Are Negotiable, Bugs Are Not =====

==== Knowledge ====
Not all feedback blocks merge.

==== Priority ====

Bugs → Must fix

Architecture → Must fix

Test refactor → Follow-up OK

==== Why Reviewers Care ====

Shipping value matters

Balance quality and speed

===== Final Meta-Lessons =====
After these reviews, you learned how to:

Think like a reviewer

Predict feedback before submitting PRs

Design maintainable systems

Communicate design intent clearly