Go CodeReview关注点

时间:2021-9-18     作者:smarteng     分类: Go语言


Gofmt

在代码上运行gofmt来修复大部分形式化的代码风格问题。现在基本所有的Go项目代码都会使用gofmt。下面讨论的都是这个工具无法解决的代码风格问题。
另外一个选择是使用goimports,它是gofmt的超集,额外提供了自动删除或导入的功能。

注释语句(Comment Sentences)

详见:https://golang.org/doc/effective_go.html#commentary 。注释文档声明应该是完整的语句,即使这样会有一些冗余。这种方式使得注释被提取到godoc文档中时可以比较好的格式化。注释应该以所描述的事物名称开始,以句号结束。

// Request represents a request to run a command.
type Request struct { ...

// Encode writes the JSON encoding of req to w.
func Encode(w io.Writer, req *Request) { ...

上下文(Contexts)

context.Context内部往往包含跨越API和进程边界的安全认证、跟踪信息、超时和取消信号等信息。Go程序在整个RPC或者HTTP请求调用链函数中显式传递Context。
大部分使用Context的函数应该作为它的第一个参数:
func F(ctx context.Context, /*other arguments*/) {}
不要把Context作为struct类型的成员变量,而是应该为类型的方法增加一个参数单独传递。有一种例外情况是在必须匹配标准库或者第三方库中的接口方法的时候。
在函数签名中不要使用自定义的Context类型或者Context之外的接口。
如果你有数据需要传递,可以通过参数、指针、全局变量的方式,或者如果合适的话,也可以使用Context的值传递。
Contexts是不可变的,可以将Context传递给多个调用,共享相关超时、取消、安全认证等信息。

拷贝(Copying)

当拷贝其他包的struct时要小心避免预期外的别名的问题。举个例子,bytes.Buffer类型中包含一个[]byte,如果你拷贝一个Buffer,拷贝中的切片可能就是原数组的别名,导致后续的方法调用产生让人意外的副作用。
一般来说,如果类型T的方法是跟指针*T绑定的,不要拷贝类型T的值。

加密随机(Crypto Rand)

不要使用包math/rand来生成密钥,即使是一次性的。如果没有设置随机种子,那么生成器是完全可预测的。即使使用time.Nanoseconds()来设置种子,也只是少量的熵。应该使用包crypto/rand的Reader,如果你需要文本,打印16进制或者Base64的字符串。

import (
"crypto/rand"
// "encoding/base64"
// "encoding/hex"
"fmt"
)

func Key() string {
buf := make([]byte, 16)
_, err := rand.Read(buf)
if err != nil {
panic(err)  // out of randomness, should never happen
}
return fmt.Sprintf("%x", buf)
// or hex.EncodeToString(buf)
// or base64.StdEncoding.EncodeToString(buf)
}

声明空切片(Declaring Empty Slices)

当声明空切片时,比较好的形式是这样:
var t []string
而不是:
t := []string{}
前面一种形式声明了一个值为nil的切片,后面一种形式是声明了非nil但是长度为0的切片。在功能上两种形式是相等的——他们的lencap都为0,但是值为nil的形式更推荐。
只有在有限的情况下使用值为非nil但是长度为0的切片会更好,比如编码JSON对象(一个nil的切片会编码为null,但是[]string{}编码JSON为[])。
当设计接口时,避免区分nil切片和非nil,长度为0的切片,主要是这会造成微妙的程序错误。
更多的关于Go nil的讨论可以参考:https://www.youtube.com/watch?v=ynoY2xz-F8s

文档注释(Doc Comments)

所有顶级的,导出的命名都需要有注释文档,那些重要的非导出的函数或者类型声明也该如此。更多可以参考:
https://golang.org/doc/effective_go.html#commentary/

不要Panic(Do not Panic)

参考:https://golang.org/doc/effective_go.html#errors 。对于一般的错误处理不要使用Panic。使用error和多返回值。

错误字符串(Error Strings)

Error的字符串不应该大写开头(特定名词除外),也不要以标点结尾,因为它们经常被打印在其他上下文中。使用fmt.Errorf("something bad")而不是fmt.Errorf("Something bad"),所以log.Printf("Reading %s: %v", filename, err)格式化形式就不会中间有错误的大写字母。这对日志不适用,因为日志是显式的面向行且不会内部组合其他信息的。

例子(Examples)

当增加一个新的包,要包含使用的例子:一个可运行的例子,或者一个演示完整调用序列的简单测试。更多可参考:https://blog.golang.org/examples

goroutine生命周期(Goroutine Lifetimes)

创建协程时,需要清楚知道它什么时候退出及是否退出。
Goroutines被阻塞在channel接收者或者生产者会导致goroutine泄漏:垃圾收集器不会终止goroutine,即使channel被block且已经不可达。
即使goroutines没有泄漏,如果不妥善处理不再需要的goroutine也会导致微妙的、很难排查的问题。对已经关闭的channel发送数据会造成Panic。在结果已经不需要时修改输入会造成无谓的数据竞争。如果不妥善处理goroutine的在内存中的处理时间会造成不可预测的内存占用。
尽量使并发的代码足够简单,使得goroutine生命周期是明确的。如果这做不到的话,需要写注释或者文档来说明何时及goroutine是怎么退出的。

错误处理(Handle Errors)

参考:/https://golang.org/doc/effective_go.html#errors/
不要使用_变量忽略错误。如果一个函数返回error,检查返回的error并确保函数执行成功。处理错误,返回它,或者在真的需要的时候Panic。

导入(Imports)

除非在命名冲突的情况下,否则尽量避免重命名导入。好的包名不应该需要重命名。在命名冲突的情况下,优先考虑重命名本地包或者特定项目的导入。
导入需要用空行来分组。标准库中的包始终应该在第一组。

package main

import (
"fmt"
"hash/adler32"
"os"

"appengine/foo"
"appengine/user"

"github.com/foo/bar"
"rsc.io/goversion/version"
)

goimports会帮你这么做。

空导入(Import Blank)

如果一个包被导入只是为了它的副作用(使用语法`import _ "pkg”\`\ ),那应该只在程序的main包中或者单元测试需要时才出现。

点导入(Import Dot)

点导入的形式方便在写测试用例的时候解决循环引用导致无法测试的问题。

package foo_test

import (
"bar/testutil" // also imports "foo"
. "foo"
)

在这里例子中,测试文件不能在包foo下,因为它使用了bar/testutil,在bar/testutil中也导入了foo包。所以我们使用import .的形式来假装文件在包foo中。除了这种情况,不要在程序中使用import .。这会严重影响程序的可读性,你没办法区分一个类似命名Quxx是当前包的一个标识符,还是被导入的包。

内联错误(In-Band Errors)

在C或者类似的语言中,使用-1或null的函数返回值来表示错误或者没有结果是比较常见的,我们管这种方式叫内联错误。

// Lookup returns the value for key or "" if there is no mapping for key.
func Lookup(key string) string

// Failing to check for an in-band error value can lead to bugs:
Parse(Lookup(key))  // returns "parse failure for value" instead of "no value for key"

Go支持的多返回值提供了一种更好的解决方式。Go语言中推荐使用一个额外的返回值来告知error,可以使用error或者bool值。这个返回值必须是返回值的最后一个。

// Lookup returns the value for key or ok=false if there is no mapping for key.
func Lookup(key string) (value string, ok bool)

这防止了调用者错误的使用了函数结果。
Parse(Lookup(key)) // compile-time error
鼓励更健壮和可读性强的代码:

value, ok := Lookup(key)
if !ok {
return fmt.Errorf("no value for %q", key)
}
return Parse(value)

这个规则适用于导出的函数,对于未导出的函数也同样有用。
返回nil,0,”"和-1作为有效返回值是可以的,也就是说调用者不需要对这些返回值做特殊处理。
一些标准库中的函数,比如包strings,返回了内联错误。这能极大简化字符串操作的代码,代价是需要开发者花费更多的精力。一般来说,Go代码应该返回额外的返回值来表示error。

缩进错误流(Ident Error Flow)

为了尽量使常规代码少缩进,我们应该缩进错误处理代码并优先处理它。这样可以快速浏览常规代码路径,从而提高代码可读性。举例来说,不要这么写:

if err != nil {
// error handling
} else {
// normal code
}

而是应该这么写:

if err != nil {
// error handling
return // or continue, etc.
}
// normal code

如果在if语句中有初始化语句,应该这么写:

if x, err := f(); err != nil {
// error handling
return
} else {
// use x
}

然后可能需要把短变量声明移动到单独的行:

x, err := f()
if err != nil {
// error handling
return
}
// use x

首字母缩写词(Initialisms)
命名中的首字母缩略词或者缩写词(比如URL或者NATO)要保持相同的大小写。比如说,“URL”可以写成“URL”或者“url”(在“urlPony”,或者“URLPony”),不应该写成“Url”。举个例子:ServeHTTP而不是ServeHttp。对于多个缩写词的标识符,使用类似“xmlHTTPRequest”或者“XMLHTTPRequest”的形式。
这个规则也同样适用于“ID”,这是“Identity”的缩写,所以应该写成“appID”而不是“appId”。
使用Protobuf编译器自动生成的代码不遵循这个规则。人类写的代码应该比机器写的代码有更高的标准。

接口(Interfaces)

一般来说,Go的接口应该在使用方的包里,而不是在实现方的包里。实现方需要返回具体的类型(通常是指针或者struct):这样的话,新方法可以被加到实现中且不需要大量的重构。
不要在API的实现侧定义专门用于Mock的API,而是应该在设计接口的时候就要考虑如何可以使用实际的API实现来测试。
不要在使用前就定义接口:如果没有实际的使用场景,是很难确定是否需要接口的,更不用说应该包含哪些方法了。

package consumer  // consumer.go

type Thinger interface { Thing() bool }

func Foo(t Thinger) string { … }
package consumer // consumer_test.go

type fakeThinger struct{ … }
func (t fakeThinger) Thing() bool { … }
…
if Foo(fakeThinger{…}) == "x" { … }
// DO NOT DO IT!!!
package producer

type Thinger interface { Thing() bool }

type defaultThinger struct{ … }
func (t defaultThinger) Thing() bool { … }

func NewThinger() Thinger { return defaultThinger{ … } }

应该返回具体的类型,让消费者来mock生产者的实现。

package producer

type Thinger struct{ … }
func (t Thinger) Thing() bool { … }

func NewThinger() Thinger { return Thinger{ … } }

行长度(Line Length)

在Go代码中没有严格的行长度限制,但要避免让人不舒服的长行。类似的,不要为了增加换行而破环可读性——比如说,如果他们是重复的。
大多数非自然的换行(在函数调用或者声明过程中)都是不必要的,只要有合理的参数数量和短变量命名就好。长的代码行似乎总是伴随着长命名,因此尽量减少长命名能有效减少长行。
换句话说,因为代码实际的语义而换行,而不是因为行的长度而换行。如果你发现这样导致行太长,那需要改变命名或者语义。
对于一个函数有多少行也是同样的建议。没有“一个函数必须小于多少行”的规则,但确实会存在很长的函数或者重复的小函数,在这种情况下我们应该改变函数的边界,而不是开始数行数。

混合大小写(Mixed Caps)

参考:https://golang.org/doc/effective_go.html#mixed-caps/
这种方式会违反在其他语言中的约定。比如未导出的常量应该是maxLength,而不是MaxLength或者MAX_LENGTH

命名结果参数(Named Result Parameters)

考虑下面代码在godoc中看起来会是怎么样。命名结果参数比如:

func (n *Node) Parent1() (node *Node) {}
func (n *Node) Parent2() (node *Node, err error) {}

在godoc中会重复,比较好的形式是这样:

func (n *Node) Parent1() *Node {}
func (n *Node) Parent2() (*Node, error) {}

另一方面来说,如果一个函数返回两个或者三个相同类型的返回值,或者结果的含义在上下文中不明确,那么增加命名在这些情况下是有帮助的。不要只是为了避免在函数中声明var而为返回值参数命名,这样会以不必要的API冗长来换取小小的实现简便性。
func (f *Foo) Location() (float64, float64, error)
这样会更清晰:

// Location returns f's latitude and longitude.
// Negative values mean south and west, respectively.
func (f *Foo) Location() (lat, long float64, err error)

naked return在函数行数比较少的时候是OK的。一旦是中等大小的函数,需要明确函数的返回值。不值得为了使用naked return而命名返回参数。清晰的文档永远比函数少些1、2行代码来的更重要。
最后,在某些情况下需要在defer闭包中修改返回值,这样使用命名返回值是合理的。

裸返回(Naked Returns)

返回语句没有参数会返回命名的返回值。这就是所谓的“naked”返回。

func split(sum int) (x, y int) {
x = sum * 4 / 9
y = sum - x
return
}

包注释(Package Comments)
包注释,与所有在godoc中的展示的注释类似,必须邻近package语句,且没有空行。

// Package math provides basic constants and mathematical functions.
package math
/*
Package template implements data-driven templates for generating textual
output such as HTML.
....
*/
package template

对于main包的注释,其他类型的注释可以放在二进制文件名之后(如果是第一个的话它一般是大写字母开头的),比如说,在seedgen文件夹下的package main可以这么写:

// Binary seedgen ...
package main

或者

// Command seedgen ...
package main

或者

// Program seedgen ...
package main

或者

// The seedgen command ...
package main

或者

// The seedgen program ...
package main

或者

// Seedgen ..
package main

这些是例子,合理的变种也是可以接收的。
小写字母单词开头的语句在包注释中是不可接受的,由于它是公共可见的,必须使用规范的英语,包括首个单词首字母大写。当二进制的名字为第一个单词时,即使它与命令行调用的拼写不严格匹配,也应该将其大写。
更多注释约定参考:https://golang.org/doc/effective_go.html#commentary/

包命名(Package Names)

对包中所有名称的引用都将使用包名,因此可以从标识符中省略该名称。举例来说,比如在包chubby中,你不需要ChubbyFile类型,这样调用端会这么写:chubby.ChubbyFile。应该使用File类型,这样客户端只需要这么写:chubby.File。避免无意义的包名,比如util,common,misc,api,types和interfaces。更多可参考:http://golang.org/doc/effective_go.html#package-names 和http://blog.golang.org/package-names 。

值传递(Pass Values)

不要将指针作为参数传给函数只是为了节省几个字节。如果函数自始自终只是将参数x引用为x,那么参数就不应该为指针。常见的例子还包括传递string(string)或者接口类型(*io.Reader)的指针。在这些例子中它们本身的值都是固定大小的所以可以直接传递。这个建议不适用于巨大的struct,甚至小的struct也会增长。

接收者命名(Receiver Names)

方法接收者的命名需要能反映它的标识。通常1-2个字母的缩写就够了(比如c或者cl标识client)。不要使用通用的命名比如
me、this、self,这些在面向对象的语言中赋予方法特殊含义的标识符。在Go中,方法接收者只是另一个参数,因此只需要正常命名。命名不需要像方法参数那样具有描述性,因为他的职责是确定的,也没有用于文档的需要。由于它基本会出现在该类型每个方法的每行代码,因此应该是很短的,这样会更简洁。为了保持一致,如果在一个方法中接收者命名为c,那么不要再另外一个方法中命名为cl。

接收者类型(Receiver Type)

选择使用指针还是值作为方法接收者是很困难的,特别是对于新的Go程序员。如果不确定,就使用指针,在有些情况下使用值接收者是合理的,通常是为了性能,比如对于一些小的不可变的struct或者基本类型的值。一些有用的原则:

  • 如果接收者是map、func或者chan,不要使用指针。如果接收者是切片并且方法不会reslice切片,不要使用指针。
  • 如果方法需要改变接收者,那么接收者必须是指针。
  • 如果接收者是struct并且包含了sync.Mutex或者类似的同步字段,接收者必须是指针避免拷贝。
  • 如果接收者是巨大的struct或者数组,指针接收者会更高效。多大算巨大呢?假设它等价于将其所有元素作为参数传递给方法。如果这样感觉太大了,那对接收者来说也就太大了。
  • 函数或者方法,无论并发调用还是从方法中调用,是否会修改接收者的值?值类型的接收者方法被调用时创建了一个接收者的拷贝,所以外部的更新不会对接收者生效。如果改变必须对原始接收者生效,那么接收者必须是指针。
  • 如果接收者是struct,数组或者切片或者其他含有指针元素的结构时,优先使用指针接收者,这样可以让读者更清晰的理解意图。
  • 如果接收者是一个小数组或者天生是值类型的struct时(比如time.Time类型),没有可变的字段和指针,或者只是一个简单的基本型比如int或者string,值接收者是合理的。值接收者能减少垃圾产生的数量。如果一个值被传递给值方法,那么这样会在栈上分配内存而不是在堆上(编译器会尽量避免这样的分配,但不是总会成功)。不要在profile前因为这个原因而选择值接收者类型。
  • 不要混合使用值和指针接收者类型。对所有的方法选择值或者指针接收者类型。
  • 最后,如果不确定,使用指针接收者。

同步函数(Synchronous Functions)

能用同步函数就不要用异步函数。同步函数的定义是指在函数返回前完成所有的回调和channel操作,直接返回结果。
同步函数保持gotoutine的本地性,使得gotoutine的声明周期更清晰,不容易产生泄漏和数据竞争。这也更方便测试:调用者只要传入入参,检查返回值,而不需要等待同步。
如果调用者需要更高的并发,只需要在单独的goroutine中执行。但这是很困难的,有时候是不可能的,在调用侧去除不必要的并发。

有用的测试失败(Useful Test Failures)

测试失败时需要有有用的信息表示哪部分失败了,在这个输入下,实际结果是什么,期望的结果是什么。编写一堆assertFoo帮助程序可能很诱人,但要确保帮助程序生成有用的信息。假设debug你的失败测试的不是你自己,也不是你的团队。一个典型的Go测试失败是这样的:

if got != tt.want {
t.Errorf("Foo(%q) = %d; want %d", tt.in, got, tt.want) // or Fatalf, if test can't test anything more past this point
}

记住这里的顺序是actual != expected,message也是这个顺序。一些测试框架鼓励反过来写,0 != x, "expected 0, got x"。Go不是这样的。
如果这样看起来代码量太大,可以使用表驱动测试 https://github.com/golang/go/wiki/TableDrivenTests
当使用不同输入的测试时,另一种消除失败歧义的方式是是用不用的TestFoo函数包装每个调用者,这样测试用例会以该名称失败。

func TestSingleValue(t *testing.T) { testHelper(t, []int{80}) }
func TestNoValues(t *testing.T)    { testHelper(t, []int{}) }

在任何情况下,你都有责任为未来调试你代码的人提供有用的信息。

变量命名(Variable Names)

在Go中变量命名应该尽量简短。特别是对于本地变量。c比lineCount好,i比sliceIndex好。
基本原则:变量使用的地方离声明的地方越远,命名需要越具体。对于方法接收者,1-2个字母足够了。通常的循环下标、读取器索引使用i,r足够。更多不常见的和全局变量需要更具体的命名。

标签: code-review