Why Code Review Matters: Core Principles for Better Backend Development

Preface

As a member of the company’s Golang code‑review committee, I have examined many code reviews and comments, and I have found that many developers need to improve both their review skills and the quality of the code they write. I would like to share some of my ideas and thoughts.

Why technical staff, including leaders, should do code review

The proverb says, "Talk is cheap, show me the code." Knowing a design principle is easy; applying it is hard. Many people only claim to understand a concept without actually practicing it, leading to a false sense of security.

Code is where design ideas become reality. During review, we can discuss concrete implementations instead of abstract ideas, learn from each other, and converge on the best practices for the team. Even if a leader does not write code, they should still point out poor practices and suggest better alternatives.

Why reviewers should think and summarize best practices

In my view, an architect masters many design ideas and principles, applies them across languages and toolchains, and defines engineering standards that keep a 300k‑line codebase maintainable, testable, and operationally sound.

Skilled engineers can be grouped into several directions:

Tricks and clever techniques – useful for contests but often not for production.

Domain foundations – e.g., John Carmack’s graphics rendering breakthroughs.

Theoretical research – e.g., Li Kaifu’s early speech‑recognition system.

Product success – practical implementations.

Best practices – the most attainable goal for any team.

Improving as an engineer means continuously refining best‑practice methodologies and implementation details.

Root causes of bad code

Before discussing good code, we must define bad code. Many problems are self‑inflicted.

Duplicated 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}
    // ...
}

When the protocol is poorly designed, each new consumer rewrites similar request‑building code, leading to duplicated fragments that must be updated everywhere when the protocol changes.

Choosing between two similar functions or RPC endpoints becomes a "life dilemma"; the decision often indicates the code is already heading toward decay.

Early decisions become ineffective later

Initial implementations may be fine, but as requirements evolve, the original design can become a bottleneck.

// Update 增量更新
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 {
    info, err := s.Get(key)
    if err != nil { return err }
    incOnlyModify := update(info, &key, clear, subtract, increment, policy, adv, shareKey, importQQGroupID)
    stat := statAndUpdateAccessInfo(info)
    if !incOnlyModify {
        if stat.groupNumber > model.FilePrivilegeGroupMax {
            return errors.Errorf(errors.PrivilegeGroupLimit, "group num %d larger than limit %d", stat.groupNumber, model.FilePrivilegeGroupMax)
        }
    }
    if !isMerge {
        if key.DomainID == uint64(access.SPECIAL_FOLDER_DOMAIN_ID) && len(info.AccessInfos) > model.FilePrivilegeMaxFolderNum {
            return errors.Errorf(errors.PrivilegeFolderLimit, "folder owner num %d larger than limit %d", len(info.AccessInfos), model.FilePrivilegeMaxFolderNum)
        }
        if len(info.AccessInfos) > model.FilePrivilegeMaxNum {
            return errors.Errorf(errors.PrivilegeUserLimit, "file owner num %d larger than limit %d", len(info.AccessInfos), model.FilePrivilegeMaxNum)
        }
    }
    pbDataSt := infoToData(info, &key)
    var updateBuf []byte
    if updateBuf, err = proto.Marshal(pbDataSt); err != nil {
        return errors.Wrapf(err, errors.MarshalPBError, "FilePrivilegeStore.Update Marshal data error, key[%v]", key)
    }
    if err = s.setCKV(generateKey(&key), updateBuf); err != nil {
        return errors.Wrapf(err, errors.Code(err), "FilePrivilegeStore.Update setCKV error, key[%v]", key)
    }
    return nil
}

When additional logic expands the function beyond 50 lines, the reader’s mental cache is overwhelmed, making it hard to follow the top‑level flow.

Premature optimization

Often discussed, but omitted here for brevity.

Lack of rigor in choosing implementations

Statements like "both ways are fine, pick any" undermine code quality.

// Get 获取IP
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 defer for unlocking is clearer and less error‑prone.

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‑reliance on inheritance / encapsulation

Excessive inheritance trees make it hard for junior engineers to modify code safely; composition is often a better alternative.

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

Deep inheritance hierarchies obscure intent and increase cognitive load.

No design at all

When developers start coding without any design, the resulting codebase becomes a tangled mess that is hard for others to maintain.

Metaphysical thinking

Beyond technical details, engineers should cultivate a higher‑level engineering mindset, connecting requirements with package and function organization, and abstracting principles from concrete experiences.

Model design

Designing models without understanding standards (e.g., OAuth2.0 RFC) leads to reinvented, fragile solutions.

Unix design philosophy

"Those who do not understand Unix will inevitably reinvent a broken Unix." – Henry Spencer, 1987.

Key principles include:

Keep It Simple Stupid (KISS)

True simplicity means a concise, complete design, not merely a short implementation.

Composition principle

Prefer composition over inheritance to keep modules interchangeable and easier to evolve.

Frugality principle

Write the smallest program that satisfies the requirement; avoid unnecessary bloat.

Transparency principle

Design should be visible and auditable, enabling reviewers to understand behavior quickly.

Common‑sense interface principle

Avoid overly clever or unconventional APIs that increase learning cost.

Silence principle

Only emit logs when they provide actionable information; excessive logging drowns out real issues.

Remediation principle

When an exception occurs, abort early and provide detailed error information.

Specific practice points (for Golang)

Enforce code formatting 100%.

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

Functions should stay under 80 lines; refactor when they exceed.

Nesting depth should not exceed four levels; use early returns.

Maintain clear directory, package, file, struct, and function hierarchies.

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

Refactor locally when old code violates new principles.

Separate generic code into its own repository; keep project‑specific code together.

If a reviewed snippet is unclear, seek clarification from senior engineers.

Log sparingly and include essential context.

Ask questions immediately; no fear of “stupid” queries.

Present issues as questions, not suggestions.

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

Eliminate duplication relentlessly.

Mainline development

Reviews should stay under 500 lines to ensure thorough examination; larger changes become superficial and harder to correct.

Continuous integration (CI) resources are abundant; engineers should study them.

"The Art of Unix Programming"

Reading this book is recommended for senior engineers to deepen their understanding of enduring engineering principles that transcend rapid technological change.

Many modern engineering methods become obsolete, but the timeless wisdom in this book remains relevant.