Effective Code Review and Engineering Best Practices

This article discusses the importance of code review as the bridge between design ideas and concrete implementation. It argues that without practical review, developers may only claim to understand design principles without actually applying them, leading to hidden technical debt.

It emphasizes that code review should focus on concrete, executable code rather than abstract discussions. Reviewers and authors need to communicate the rationale behind design choices and ensure that the code reflects best‑practice principles.

Several Go code examples illustrate common pitfalls and improvements:

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

The author points out that early protocol design flaws can cause duplicated implementations across teams, making future refactoring painful.

Another example shows a large update function whose length and nested logic make it hard to read. The article suggests splitting files (max 800 lines), functions (max 80 lines), and limiting nesting depth to four levels, using early returns to simplify control flow.

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

To avoid duplicated logic, the author recommends extracting common patterns into reusable utilities and applying the "early return" style:

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

which can be simplified to:

if !needContinue {
    doA()
    return
}

doB()
return

The article also critiques over‑engineered object‑oriented designs, especially deep inheritance hierarchies that obscure logic. It advocates composition over inheritance, clear package structures, and keeping each layer of abstraction transparent.

Key engineering principles highlighted include:

Strict code formatting (100% compliance).

File size limit of 800 lines; split larger files.

Function size limit of 80 lines; split larger functions.

Maximum nesting depth of four levels; prefer early returns.

Eliminate duplicated code wherever possible.

Use multi‑level directories to reflect logical organization.

Log minimally but include essential context.

Ask questions directly during review; avoid vague suggestions.

The author recommends classic literature such as "The Art of Unix Programming" and "Software Engineering at Google" for deeper insight into timeless engineering principles.