Principles and Practices for Effective Code Review and Software Engineering

Preface

As a member of the Golang sub‑committee of the company’s code committee, I have reviewed many pieces of code and comments, and I found that many engineers need to improve both their code‑review skills and their ability to write good code. This article shares my thoughts and approaches.

Why Technical Staff and Leaders Should Do Code Review

Talking is cheap; showing code is essential. Knowing a design principle is not enough; you must practice it. Code is the concrete manifestation of design ideas, and a thorough review turns abstract discussion into practical improvement. Even leaders who do not write code should still provide concrete feedback on others’ implementations.

Why Reviewers Must Think About Best Practices

Architects master many design principles across languages and toolchains, enabling them to keep large projects (hundreds of thousands of lines) maintainable. Engineers can be categorized into several directions, such as clever tricks, foundational work, theoretical research, product success, and best‑practice implementation.

Root Causes of Bad Code

Duplicate Code

When the initial protocol design is poor, each developer may re‑implement similar request‑building logic, leading to many duplicated fragments that are hard to refactor and prone to bugs.

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

Early Decisions Lose Effectiveness

Initially well‑written functions can become tangled when additional logic (e.g., merge handling) expands them beyond a readable size, forcing reviewers to jump between contexts and increasing cognitive load.

// 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)
        }
    }
    // marshal and store …
    return nil
}

Poor Optimization Timing

Premature optimization is mentioned but not elaborated; the point is to avoid it.

Lack of Rigor in Reasoning

Statements like “both ways are ok, pick any” hinder code quality because they ignore the need for clear, justified decisions.

Over‑use of OOP and Encapsulation

Excessive inheritance hierarchies make it hard for newcomers to understand code. Favor composition over inheritance, especially in large, loosely coupled teams.

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

Fundamental Principles

Keep It Simple Stupid (KISS)

Simplicity is not just the first obvious solution; it requires deep understanding of the problem and continuous refinement.

Composition Principle

Design modules as composable parts rather than deep inheritance trees; this reduces mental overhead and improves extensibility.

type Request struct {
    Method string
    URL    *url.URL
    Proto  string // "HTTP/1.0"
    ProtoMajor int
    ProtoMinor int
    Header Header
    Body   io.ReadCloser
    // … other fields …
}

Transparency Principle

Code should be easy to read and reason about; variables and function names must convey intent, and documentation should be close to the code.

Frugality Principle

Write the smallest program that works; avoid unnecessary bloat, as larger codebases become harder to maintain.

Explicitness Principle

Avoid “clever” naming that obscures meaning; use conventional names (e.g., X, Y instead of VerticalOrdinate).

type Point struct { X float64; Y float64 }
// vs.
type Point struct { VerticalOrdinate float64; HorizontalOrdinate float64 }

Silence Principle

Log only essential information; excessive logging drowns out real problems.

Remedy Principle

When an exception occurs, abort early and provide sufficient diagnostic data.

Practical Guidelines for Golang Projects

Enforce code‑formatting 100%.

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

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

Nesting depth should be ≤ 4; use early returns to flatten logic.

if !needContinue {
    doA()
    return
}
// …

Model Design

Good model design reduces future refactoring pain. Use domain‑driven design to keep interfaces stable and to separate concerns, enabling teams to evolve features without massive rewrites.

Unix Design Philosophy

Software should be transparent, composable, and minimal. The classic “Unix Programming Art” book remains relevant for modern engineers.

Specific Practices

Strictly follow formatting rules.

Limit file and function size.

Keep nesting shallow.

Organize code in hierarchical packages and directories.

Refactor legacy code promptly.

Log minimally but with key identifiers.

Ask questions early; do not fear “stupid” queries.

Use trpc and align with leadership on quality standards.

Eliminate duplication aggressively.

Trunk‑Based Development

Keep review size under 500 lines to ensure thorough inspection; larger changes become error‑prone and harder to adjust.

Conclusion

Read “The Unix Programming Art” for timeless engineering wisdom, internalize its principles, and continuously apply them in daily code‑review practice.