騰訊 Code Review 規(guī)范出爐！

前言

為什么技術人員包括 leader 都要做 code review

為什么同學們要在 review 中思考和總結最佳實踐

• 奇技淫巧

• 領域奠基

• 理論研究

• 產品成功

• 最佳實踐

代碼變壞的根源

重復的代碼

// BatchGetQQTinyWithAdmin 獲取QQ uin的tinyID, 需要主uin的tiny和登錄態(tài)
// 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?{
?// 獲取之前的數(shù)據(jù)
?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
}

過早的優(yōu)化

對合理性沒有苛求

// 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 補救原則: 出現(xiàn)異常時，馬上退出并給出足夠錯誤信息

具體實踐點

• 對于代碼格式規(guī)范，100%嚴格執(zhí)行，嚴重容不得一點沙。

• 文件絕不能超過 800 行，超過，一定要思考怎么拆文件。工程思維，就在于拆文件的時候積累。

• 函數(shù)對決不能超過 80 行，超過，一定要思考怎么拆函數(shù)，思考函數(shù)分組，層次。工程思維，就在于拆文件的時候積累。

• 代碼嵌套層次不能超過 4 層，超過了就得改。多想想能不能 early return。工程思維，就在于拆文件的時候積累。

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

doB()
return

• 從目錄、package、文件、struct、function 一層層下來 ，信息一定不能出現(xiàn)冗余。比如 file.FileProperty 這種定義。只有每個'定語'只出現(xiàn)在一個位置，才為'做好邏輯、定義分組/分層'提供了可能性。
• 多用多級目錄來組織代碼所承載的信息，即使某一些中間目錄只有一個子目錄。
• 隨著代碼的擴展，老的代碼違反了一些設計原則，應該立即原地局部重構，維持住代碼質量不滑坡。比如:拆文件；拆函數(shù)；用 Session 來保存一個復雜的流程型函數(shù)的所有信息；重新調整目錄結構。
• 基于上一點考慮，我們應該盡量讓項目的代碼有一定的組織、層次關系。我個人的當前實踐是除了特別通用的代碼，都放在一個 git 里。特別通用、修改少的代碼，逐漸獨立出 git，作為子 git 連接到當前項目 git，讓 goland 的 Refactor 特性、各種 Refactor 工具能幫助我們快速、安全局部重構。
• 自己的項目代碼，應該有一個內生的層級和邏輯關系。flat 平鋪展開是非常不利于代碼復用的。怎么復用、怎么組織復用，肯定會變成'人生難題'。T4-T7 的同學根本無力解決這種難題。
• 如果被 review 的代碼雖然簡短，但是你看了一眼卻發(fā)現(xiàn)不咋懂，那就一定有問題。自己看不出來，就找高級別的同學交流。這是你和別 review 代碼的同學成長的時刻。
• 日志要少打。要打日志就要把關鍵索引信息帶上。必要的日志必須打。
• 有疑問就立即問，不要怕問錯。讓代碼作者給出解釋。不要怕問出極低問題。
• 不要說'建議'，提問題，就是剛，你 pk 不過我，就得改！
• 請積極使用 trpc。總是要和老板站在一起！只有和老板達成的對于代碼質量建設的共識，才能在團隊里更好地做好代碼質量建設。
• 消滅重復！消滅重復！消滅重復！

主干開發(fā)

《unix 編程藝術》