Mastering Code Review: 12 Principles for Cleaner, Maintainable Code

As a member of the Golang sub‑committee of the company code review board, I have reviewed many pieces of code and comments, noticing that many engineers and leaders still need to improve their code review skills and the quality of the code they produce. This article shares my thoughts and approaches.

Why technical staff and leaders must 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 it enables concrete communication, shared learning, and the adoption of the team’s best practices. Even leaders who do not write code should still provide thoughtful feedback on others’ implementations.

Why engineers should think and summarize best practices during review

An architect masters many design ideas, principles, and language‑specific practices, and can manage large codebases (hundreds of thousands of lines) with maintainability, testability, and operational quality. By continuously refining best‑practice principles and applying them, engineers evolve from writing ad‑hoc code to becoming architects.

Tricks and clever hacks (often useful in contests but less so in production)

Foundational domain knowledge (e.g., John Carmack’s graphics rendering breakthroughs)

Theoretical research (e.g., Li Kaifu’s early use of hidden Markov models for speech recognition)

Product success stories

Best practices

Root causes of bad code

Repeated 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 was poorly designed, each new developer rewrote similar request‑building code, leading to duplicated fragments that are hard to refactor and error‑prone.

Early decisions become ineffective

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

Even a clean 80‑line function becomes hard to understand when nested logic (e.g., the isMerge branch) expands beyond 50 lines, forcing reviewers to jump between levels and lose context.

Premature optimization

Often mentioned but not repeated here.

Lack of rigor in reasoning

Statements like "both ways are ok, pick any" hinder code quality because they avoid critical evaluation of alternatives.

No design at all

When code is written without any upfront design, it becomes a tangled mess that is difficult for others to maintain.

Must think metaphysically

Beyond learning isolated tricks, engineers should build a layered mental model that connects low‑level details to high‑level system requirements, enabling them to design robust models and interfaces.

Model design

Designing models based on solid domain understanding prevents costly rewrites when requirements change. For example, a naive key‑value calendar design quickly breaks when scaling to millions of users or supporting group events.

UNIX design philosophy

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

Keep It Simple Stupid!

The KISS principle is well known, but true simplicity means designing concise, complete solutions rather than merely short code snippets.

Composition principle: design for assembly

Instead of deep inheritance trees, use composition to build flexible modules (e.g., Go’s http.Request struct). This reduces cognitive load and eases collaboration across seniority levels.

// A Request represents an HTTP request received by a server or to be sent by a client.
type Request struct {
    Method string
    URL    *url.URL
    Proto      string // "HTTP/1.0"
    ProtoMajor int    // 1
    ProtoMinor int    // 0
    Header Header
    Body   io.ReadCloser
    GetBody func() (io.ReadCloser, error)
    ContentLength int64
    TransferEncoding []string
    Close bool
    Host string
    Form url.Values
    PostForm url.Values
    MultipartForm *multipart.Form
    Trailer Header
    RemoteAddr string
    RequestURI string
    TLS *tls.ConnectionState
    Cancel <-chan struct{}
    Response *Response
    ctx context.Context
}

Understanding such composable structures helps avoid deep inheritance pitfalls seen in large OOP frameworks.

Principle 3: Composition over inheritance

Using composition lets each developer work on isolated packages without needing to understand the entire inheritance hierarchy, improving maintainability.

Principle 6: Be parsimonious

Write the smallest program that solves the problem; avoid bloated code that becomes hard to evolve.

Principle 7: Transparency

Code should be visible and understandable so reviewers can quickly grasp its behavior; clear naming and documentation increase transparency.

Principle 10: Avoid novelty for its own sake

Design APIs with familiar, intuitive names (e.g., use X and Y for coordinates rather than obscure terms) to reduce learning cost.

Principle 11: Silence is golden

Log only essential information; excessive logging obscures real issues.

Principle 12: Fail fast with clear errors

When an exception occurs, surface it immediately with sufficient context to aid debugging.

Concrete practice points (Golang)

Enforce code formatting 100 %.

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

Functions should stay under 80 lines; refactor otherwise.

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

if !needContinue {
    doA()
    return
}
doB()
return

Early returns decouple logic and reduce nesting.

Keep definitions non‑redundant across packages, directories, and structs.

Organize code hierarchically with meaningful directories and packages.

Refactor locally when old code violates current principles.

Use a single Git repository for most code; extract highly reusable components into separate repos.

Ask questions immediately when something is unclear; treat review as a growth opportunity.

Log minimally but include key identifiers.

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

Eliminate duplication relentlessly.

Mainline development

Reviews should stay under 500 lines to remain effective; larger changes become hard to review and risk quality.

Continuous Integration (CI) practices are also essential for maintaining code health.

Recommended reading

Read "The Art of Unix Programming" for timeless engineering wisdom, and "Software Engineering at Google" for modern large‑scale practices.