Principles and Practices for Effective Code Review and Software Architecture

Preface

As a member of the company’s Golang sub‑committee, I have reviewed many pieces of code and comments, and I found that many developers need to improve their code‑review skills and overall code quality. I would like to share some of my ideas and thoughts here.

Why Technical Staff, Including Leaders, Should Do Code Review

‘Talk is cheap, show me the code.’ Knowing a design principle is easy; applying it in practice is hard. Code is the concrete manifestation of design ideas, and reviewing code allows concrete communication, learning, and the adoption of the best practices accumulated in the team.

Why Developers Should Summarize Best Practices During Review

An architect masters many design ideas, principles, language‑specific practices, and industry models, and can control the maintainability, testability, and operational quality of a 300k‑line project.

Tricks and clever hacks

Mastering many tricks is useful for contests but not for engineering.

Domain foundations

John Carmack invented modern computer graphics rendering methods; such groundbreaking work is rare.

Theoretical research

Li Kaifu’s early speech recognition system is an example of pioneering research.

Product success

‘Little Dragon’ is a benchmark.

Best practice

Following the definition of an architect, building high‑quality systems for large teams is the goal.

Root Causes of Bad Code

Before discussing good code, we first discuss what makes code bad.

Duplicate Code

// BatchGetQQTinyWithAdmin 获取QQ uin的tinyID, 需要主uin的tiny和登录态
// friendUins 可以是空列表, 只要admin uin的tiny
func BatchGetQQTinyWithAdmin(ctx context.Context, adminUin uint64, friendUin []uint64) (adminTiny uint64, sig []byte, frdTiny map[uint64]uint64, err error) {
    var friendAccountList []*basedef.AccountInfo
    for _, v := range friendUin {
        friendAccountList = append(friendAccountList, &basedef.AccountInfo{AccountType: proto.String(def.StrQQU), Userid: proto.String(fmt.Sprint(v))})
    }
    req := &cmd0xb91.ReqBody{Appid: proto.Uint32(model.DocAppID), CheckMethod: proto.String(CheckQQ), AdminAccount: &basedef.AccountInfo{AccountType: proto.String(def.StrQQU), Userid: proto.String(fmt.Sprint(adminUin))}, FriendAccountList: friendAccountList}
    // ...
}

Because the initial protocol design was poor, each developer rewrote similar request‑building code, leading to duplicated fragments that are hard to maintain and evolve.

Early Decisions Lose Effectiveness

func (s *FilePrivilegeStore) Update(key def.PrivilegeKey, clear, isMerge bool, subtract []*access.AccessInfo, increment []*access.AccessInfo, policy *uint32, adv *access.AdvPolicy, shareKey string, importQQGroupID uint64) error {
    // get previous data
    info, err := s.Get(key)
    if err != nil { return err }
    // ... many lines of logic ...
    if !isMerge {
        if stat.groupNumber > model.FilePrivilegeGroupMax {
            return errors.Errorf(errors.PrivilegeGroupLimit, "group num %d larger than limit %d", stat.groupNumber, model.FilePrivilegeGroupMax)
        }
    }
    // ... more logic ...
    return nil
}

The function grows beyond 80 lines, mixing top‑level logic with deep inner details, making it hard to read and maintain.

Premature Optimization

Commonly heard but not elaborated here.

Lack of Rigor

‘Both ways are OK, pick any.’ Such lax attitudes lead to ambiguous implementations.

func (i *IPGetter) Get(cardName string) string {
    i.l.RLock()
    ip, found := i.m[cardName]
    i.l.RUnlock()
    if found { return ip }
    i.l.Lock()
    var err error
    ip, err = getNetIP(cardName)
    if err == nil { i.m[cardName] = ip }
    i.l.Unlock()
    return ip
}

Using explicit unlocks is error‑prone; a defer would be safer.

i.l.Lock()
defer i.l.Unlock()
var err error
ip, err = getNetIP(cardName)
if err != nil { return "127.0.0.1" }
i.m[cardName] = ip
return ip

Over‑use of OOP / Encapsulation

Excessive inheritance trees make code hard to understand; composition is preferred.

template
class CSuperAction : public CSuperActionBase {
public:
    typedef _PKG_TYPE pkg_type;
    typedef CSuperAction
this_type;
    // ...
};

Such deep hierarchies hinder readability and reuse.

Metaphysical Thinking

Beyond technical details, engineers should develop a higher‑level thinking model that connects code details with system requirements.

Model Design

Designing without understanding standards (e.g., OAuth2.0) leads to reinventing flawed solutions.

UNIX Design Philosophy

Quote: “Those who do not understand Unix are doomed to reinvent a broken Unix.”

Keep It Simple Stupid!

True simplicity requires deep understanding of the problem, not just a superficial solution.

Composition Principle

Prefer composition over inheritance; break systems into small, combinable parts.

type Request struct {
    Method string
    URL *url.URL
    Proto string
    ProtoMajor int
    ProtoMinor int
    Header Header
    Body io.ReadCloser
    // ... many fields omitted for brevity ...
}

The Go standard library’s HTTP request struct demonstrates a well‑composed model.

Frugality Principle

Write the smallest program that works; avoid unnecessary bloat.

Transparency Principle

Design should be visible for review and debugging; clear variable and function names improve understandability.

Common‑Sense Principle

Avoid overly clever interfaces; keep APIs intuitive.

Concrete Practice Points (Golang‑Specific)

Enforce code‑format standards 100%.

Files must not exceed 800 lines; split when they do.

Functions must not exceed 80 lines; refactor when they do.

Nesting depth must not exceed 4 levels; use early returns.

if !needContinue {
    doA()
    return
}
// else omitted – early return simplifies flow

doB()
return

Early returns decouple logic branches.

Organize code hierarchically: directory → package → file → struct → function.

Use multi‑level directories even if some levels have a single child.

When old code violates principles, refactor locally immediately.

Prefer composition over inheritance for extensibility.

If a reviewed snippet is hard to understand, ask senior engineers.

Log sparingly; include essential context.

Ask questions promptly; do not fear “low‑level” queries.

Do not merely suggest changes; raise concrete issues that must be addressed.

Adopt trpc and align with leadership on code‑quality goals.

Eliminate duplication relentlessly.

Main‑line Development

Keep review size under 500 lines to ensure thorough inspection and easier adjustments.

Recommended Reading

Read “The Art of UNIX Programming” and “Software Engineering at Google” for timeless engineering wisdom.