Engineering Knowledge Learned from PR Reviews

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