腾讯 Code Review 规范出炉！
阅读本文大概需要 30 分钟。
来自：腾讯技术工程，作者：cheaterlin，腾讯 PCG 后台开发工程师
tips：长文警告！！！
前言
为什么技术人员包括 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<pkg_type> 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 编程艺术》
评论