腾讯 Code Review 规范出炉！

前言

为什么技术人员包括 leader 都要做 code review

为什么同学们要在 review 中思考和总结最佳实践

• 奇技淫巧

• 领域奠基

• 理论研究

• 产品成功

• 最佳实践

代码变坏的根源

重复的代码

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

早期有效的决策不再有效

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

过早的优化

对合理性没有苛求

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

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

总是面向对象/总喜欢封装

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

根本没有设计

必须形而上的思考

model 设计

UNIX 设计哲学

Keep It Simple Stuped!

原则 3 组合原则: 设计时考虑拼接组合

// A Request represents an HTTP request received by a server
// or to be sent by a client.
//
// The field semantics differ slightly between client and server
// usage. In addition to the notes on the fields below, see the
// documentation for Request.Write and RoundTripper.
type Request struct {
	// Method specifies the HTTP method (GET, POST, PUT, etc.).
	// For client requests, an empty string means GET.
	//
	// Go's HTTP client does not support sending a request with
	// the CONNECT method. See the documentation on Transport for
	// details.
	Method string

	// URL specifies either the URI being requested (for server
	// requests) or the URL to access (for client requests).
	//
	// For server requests, the URL is parsed from the URI
	// supplied on the Request-Line as stored in RequestURI.  For
	// most requests, fields other than Path and RawQuery will be
	// empty. (See RFC 7230, Section 5.3)
	//
	// For client requests, the URL's Host specifies the server to
	// connect to, while the Request's Host field optionally
	// specifies the Host header value to send in the HTTP
	// request.
	URL *url.URL

	// The protocol version for incoming server requests.
	//
	// For client requests, these fields are ignored. The HTTP
	// client code always uses either HTTP/1.1 or HTTP/2.
	// See the docs on Transport for details.
	Proto      string // "HTTP/1.0"
	ProtoMajor int    // 1
	ProtoMinor int    // 0

	// Header contains the request header fields either received
	// by the server or to be sent by the client.
	//
	// If a server received a request with header lines,
	//
	//	Host: example.com
	//	accept-encoding: gzip, deflate
	//	Accept-Language: en-us
	//	fOO: Bar
	//	foo: two
	//
	// then
	//
	//	Header = map[string][]string{
	//		"Accept-Encoding": {"gzip, deflate"},
	//		"Accept-Language": {"en-us"},
	//		"Foo": {"Bar", "two"},
	//	}
	//
	// For incoming requests, the Host header is promoted to the
	// Request.Host field and removed from the Header map.
	//
	// HTTP defines that header names are case-insensitive. The
	// request parser implements this by using CanonicalHeaderKey,
	// making the first character and any characters following a
	// hyphen uppercase and the rest lowercase.
	//
	// For client requests, certain headers such as Content-Length
	// and Connection are automatically written when needed and
	// values in Header may be ignored. See the documentation
	// for the Request.Write method.
	Header Header

	// Body is the request's body.
	//
	// For client requests, a nil body means the request has no
	// body, such as a GET request. The HTTP Client's Transport
	// is responsible for calling the Close method.
	//
	// For server requests, the Request Body is always non-nil
	// but will return EOF immediately when no body is present.
	// The Server will close the request body. The ServeHTTP
	// Handler does not need to.
	Body io.ReadCloser

	// GetBody defines an optional func to return a new copy of
	// Body. It is used for client requests when a redirect requires
	// reading the body more than once. Use of GetBody still
	// requires setting Body.
	//
	// For server requests, it is unused.
	GetBody func() (io.ReadCloser, error)

	// ContentLength records the length of the associated content.
	// The value -1 indicates that the length is unknown.
	// Values >= 0 indicate that the given number of bytes may
	// be read from Body.
	//
	// For client requests, a value of 0 with a non-nil Body is
	// also treated as unknown.
	ContentLength int64

	// TransferEncoding lists the transfer encodings from outermost to
	// innermost. An empty list denotes the "identity" encoding.
	// TransferEncoding can usually be ignored; chunked encoding is
	// automatically added and removed as necessary when sending and
	// receiving requests.
	TransferEncoding []string

	// Close indicates whether to close the connection after
	// replying to this request (for servers) or after sending this
	// request and reading its response (for clients).
	//
	// For server requests, the HTTP server handles this automatically
	// and this field is not needed by Handlers.
	//
	// For client requests, setting this field prevents re-use of
	// TCP connections between requests to the same hosts, as if
	// Transport.DisableKeepAlives were set.
	Close bool

	// For server requests, Host specifies the host on which the
	// URL is sought. For HTTP/1 (per RFC 7230, section 5.4), this
	// is either the value of the "Host" header or the host name
	// given in the URL itself. For HTTP/2, it is the value of the
	// ":authority" pseudo-header field.
	// It may be of the form "host:port". For international domain
	// names, Host may be in Punycode or Unicode form. Use
	// golang.org/x/net/idna to convert it to either format if
	// needed.
	// To prevent DNS rebinding attacks, server Handlers should
	// validate that the Host header has a value for which the
	// Handler considers itself authoritative. The included
	// ServeMux supports patterns registered to particular host
	// names and thus protects its registered Handlers.
	//
	// For client requests, Host optionally overrides the Host
	// header to send. If empty, the Request.Write method uses
	// the value of URL.Host. Host may contain an international
	// domain name.
	Host string

	// Form contains the parsed form data, including both the URL
	// field's query parameters and the PATCH, POST, or PUT form data.
	// This field is only available after ParseForm is called.
	// The HTTP client ignores Form and uses Body instead.
	Form url.Values

	// PostForm contains the parsed form data from PATCH, POST
	// or PUT body parameters.
	//
	// This field is only available after ParseForm is called.
	// The HTTP client ignores PostForm and uses Body instead.
	PostForm url.Values

	// MultipartForm is the parsed multipart form, including file uploads.
	// This field is only available after ParseMultipartForm is called.
	// The HTTP client ignores MultipartForm and uses Body instead.
	MultipartForm *multipart.Form

	// Trailer specifies additional headers that are sent after the request
	// body.
	//
	// For server requests, the Trailer map initially contains only the
	// trailer keys, with nil values. (The client declares which trailers it
	// will later send.)  While the handler is reading from Body, it must
	// not reference Trailer. After reading from Body returns EOF, Trailer
	// can be read again and will contain non-nil values, if they were sent
	// by the client.
	//
	// For client requests, Trailer must be initialized to a map containing
	// the trailer keys to later send. The values may be nil or their final
	// values. The ContentLength must be 0 or -1, to send a chunked request.
	// After the HTTP request is sent the map values can be updated while
	// the request body is read. Once the body returns EOF, the caller must
	// not mutate Trailer.
	//
	// Few HTTP clients, servers, or proxies support HTTP trailers.
	Trailer Header

	// RemoteAddr allows HTTP servers and other software to record
	// the network address that sent the request, usually for
	// logging. This field is not filled in by ReadRequest and
	// has no defined format. The HTTP server in this package
	// sets RemoteAddr to an "IP:port" address before invoking a
	// handler.
	// This field is ignored by the HTTP client.
	RemoteAddr string

	// RequestURI is the unmodified request-target of the
	// Request-Line (RFC 7230, Section 3.1.1) as sent by the client
	// to a server. Usually the URL field should be used instead.
	// It is an error to set this field in an HTTP client request.
	RequestURI string

	// TLS allows HTTP servers and other software to record
	// information about the TLS connection on which the request
	// was received. This field is not filled in by ReadRequest.
	// The HTTP server in this package sets the field for
	// TLS-enabled connections before invoking a handler;
	// otherwise it leaves the field nil.
	// This field is ignored by the HTTP client.
	TLS *tls.ConnectionState

	// Cancel is an optional channel whose closure indicates that the client
	// request should be regarded as canceled. Not all implementations of
	// RoundTripper may support Cancel.
	//
	// For server requests, this field is not applicable.
	//
	// Deprecated: Set the Request's context with NewRequestWithContext
	// instead. If a Request's Cancel field and context are both
	// set, it is undefined whether Cancel is respected.
	Cancel <-chan struct{}

	// Response is the redirect response which caused this request
	// to be created. This field is only populated during client
	// redirects.
	Response *Response

	// ctx is either the client or server context. It should only
	// be modified via copying the whole Request using WithContext.
	// It is unexported to prevent people from using Context wrong
	// and mutating the contexts held by callers of the same request.
	ctx context.Context
}

原则 6 吝啬原则: 除非确无它法, 不要编写庞大的程序

原则 7 透明性原则: 设计要可见，以便审查和调试

原则 10 通俗原则: 接口设计避免标新立异

type Point struct {
 X float64
 Y float64
}

type Point struct {
 VerticalOrdinate   float64
 HorizontalOrdinate float64
}

原则 12 补救原则: 出现异常时，马上退出并给出足够错误信息

具体实践点

• 对于代码格式规范，100%严格执行，严重容不得一点沙。

• 文件绝不能超过 800 行，超过，一定要思考怎么拆文件。工程思维，就在于拆文件的时候积累。

• 函数对决不能超过 80 行，超过，一定要思考怎么拆函数，思考函数分组，层次。工程思维，就在于拆文件的时候积累。

• 代码嵌套层次不能超过 4 层，超过了就得改。多想想能不能 early return。工程思维，就在于拆文件的时候积累。

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

doB()
return

• 从目录、package、文件、struct、function 一层层下来 ，信息一定不能出现冗余。比如 file.FileProperty 这种定义。只有每个'定语'只出现在一个位置，才为'做好逻辑、定义分组/分层'提供了可能性。
• 多用多级目录来组织代码所承载的信息，即使某一些中间目录只有一个子目录。
• 随着代码的扩展，老的代码违反了一些设计原则，应该立即原地局部重构，维持住代码质量不滑坡。比如:拆文件；拆函数；用 Session 来保存一个复杂的流程型函数的所有信息；重新调整目录结构。
• 基于上一点考虑，我们应该尽量让项目的代码有一定的组织、层次关系。我个人的当前实践是除了特别通用的代码，都放在一个 git 里。特别通用、修改少的代码，逐渐独立出 git，作为子 git 连接到当前项目 git，让 goland 的 Refactor 特性、各种 Refactor 工具能帮助我们快速、安全局部重构。
• 自己的项目代码，应该有一个内生的层级和逻辑关系。flat 平铺展开是非常不利于代码复用的。怎么复用、怎么组织复用，肯定会变成'人生难题'。T4-T7 的同学根本无力解决这种难题。
• 如果被 review 的代码虽然简短，但是你看了一眼却发现不咋懂，那就一定有问题。自己看不出来，就找高级别的同学交流。这是你和别 review 代码的同学成长的时刻。
• 日志要少打。要打日志就要把关键索引信息带上。必要的日志必须打。
• 有疑问就立即问，不要怕问错。让代码作者给出解释。不要怕问出极低问题。
• 不要说'建议'，提问题，就是刚，你 pk 不过我，就得改！
• 请积极使用 trpc。总是要和老板站在一起！只有和老板达成的对于代码质量建设的共识，才能在团队里更好地做好代码质量建设。
• 消灭重复！消灭重复！消灭重复！

主干开发

《unix 编程艺术》