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

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

Usecase handling HTTP concerns:

if err != nil {
return makeErrorResponse(http.StatusBadRequest, "invalid request")
}

Usecase returns domain error only:

return errors.WithStack(ErrInvalidRequest)

Handler converts error to HTTP response:

if err != nil {
return http.StatusBadRequest, err.Error()
}

Usecase should work for HTTP, gRPC, batch jobs

Easier testing and reuse

Clear separation of concerns

Ask before coding:

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

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

func Execute(req *pb.GetCouponRequest) {
if req.Mode == pb.Mode_LIST {
...
}
}

Domain model:

type CouponQuery struct {
Mode CouponMode
}

Usecase:

func Execute(q CouponQuery) ([]Coupon, error) {
...
}

Handler mapping:

q := CouponQuery{
Mode: mapProtoMode(req.Mode),
}

Protobuf changes should not break business logic

Domain models should be framework-agnostic

Rule of thumb:

If pb. appears in usecase → wrong layer

Errors are signals, not just messages.

return fmt.Errorf("invalid request")

var ErrInvalidRequest = errors.New("invalid request")
 
return errors.WithStack(ErrInvalidRequest)

Stack traces in logs

Easier root-cause analysis

Error comparison using errors.Is

Always consider:

Can this error be debugged in production?

Invalid input should never reach usecase.

if req.Mode != "list" && req.Mode != "history" {
return ErrInvalidRequest
}

Declarative validation:

type Request struct {
Mode string validate:"oneof=list history"
}

Cleaner business logic

Validation rules are readable and centralized

Perform validation in:

Handler

Validator layer

Value Object constructors

Domain models represent concepts, not API fields.

CouponID
CouponCode
CouponName

ID
Code
Name

Less noise

Easier reasoning

Domain-first design

Ask:

“Would this name make sense without protobuf?”

Pointers imply mutability and shared state.

[]*Coupon

Used everywhere without reason.

[]Coupon

Use pointers only when:

Object is large

Mutation is required

Identity matters

Avoid side effects

Clear API intent

Be able to explain:

“Why is this a pointer?”

Never trust external APIs.

resp, _ := client.Do(req)
return resp.Body.Data

if resp.StatusCode != http.StatusOK {
return ErrExternalService
}
if resp.Body == nil {
return ErrInvalidResponse
}

External services fail

Partial responses happen

Always validate:

Status code

Required fields

Nil / empty values

APIs are long-term contracts.

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

Breaking changes affect other teams

Stability > aesthetics

Before changing:

Schema

API

Contract Ask:

“Who will this break?”

Responding to reviews with refactoring shows maturity.

“Out of scope”

“Existing code does this”

Refactor

Explain trade-offs

Improve architecture

Ownership mindset

Long-term thinking

Not all feedback blocks merge.

Bugs → Must fix

Architecture → Must fix

Test refactor → Follow-up OK

Shipping value matters

Balance quality and speed

After these reviews, you learned how to:

Think like a reviewer

Predict feedback before submitting PRs

Design maintainable systems

Communicate design intent clearly