Principles and Practices of Code Review and Software Architecture

As a member of the Golang sub‑committee of the company’s code committee, the author reviews many pull requests and observes that many engineers need to improve both their code‑review skills and overall code quality.

Preface

The author introduces the motivation for sharing his thoughts on code review and best‑practice thinking.

Why technical staff and leaders should do code review

Code is the concrete manifestation of design ideas; reviewing code enables concrete communication, knowledge sharing, and ensures that best practices are followed, even when leaders have little time to write code themselves.

Why engineers should think and summarize best practices

Architects master many design principles, apply them across languages and toolchains, and control large‑scale projects. The author lists several categories of technical excellence, such as clever tricks, domain foundations, theoretical research, product success, and best practices.

Root causes of bad code

Repeated code, early decisions that become obsolete, premature optimization, lack of rigor, over‑use of OOP, and absence of design all lead to fragile systems.

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

When the protocol is poorly designed, each developer rewrites similar request‑building logic, leading to duplicated, hard‑to‑maintain code.

Early decisions become invalid

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

What initially looks clean becomes tangled as more conditions are added, breaking readability and maintainability.

Poor optimization

The author notes that premature optimization is a well‑known trap and should be avoided.

Over‑encapsulation and OOP

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

Deep inheritance hierarchies increase cognitive load; composition is often a better alternative.

No design

Writing large files or functions without any prior design leads to unreadable, unmaintainable code that hinders future contributors.

Metaphysical thinking

Engineers should move from merely executing tasks to building mental models that connect technical details with system requirements, summarizing principles and applying them across projects.

Model design

Good model design prevents constant rewrites when requirements change; the author cites OAuth2, calendar apps, and domain‑driven design as examples.

UNIX design philosophy

Quotes from Henry Spencer and references to "The Art of Unix Programming" illustrate timeless engineering principles.

KISS principle

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

Composition principle

Prefer composition over inheritance to keep systems modular and adaptable.

Other principles (frugality, transparency, modesty, remediation)

Write small programs, expose clear interfaces, log only essential information, and fail fast with useful error messages.

Concrete practice points

Enforce code‑formatting strictly.

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

Functions should stay under 80 lines; refactor otherwise.

Nesting depth should be limited to four levels; use early returns.

Organize code hierarchically (directory → package → file → struct → function) without redundancy.

Use composition to reduce inheritance complexity.

Ask questions early; treat suggestions as required changes.

Adopt trpc and eliminate duplicate code.

Main‑line development

Keeping review size under 500 lines ensures thorough reviews and easier maintenance.

Recommended reading

Read "The Art of Unix Programming" and "Software Engineering at Google" to deepen engineering thinking.