Go Code review FAQ

This article, translated from the official documentation of the Go language, collects frequently asked questions about the Go language code review. It is suitable for beginners who have mastered the basic syntax.

The reading time is about 15 minutes

The original link

Gofmt

Run the Gofmt tool before submitting your code, which automatically fixes most formal problems (alignment, line breaks, and so on).

Almost all Go projects now use GoFMT, the ones they don’t because they use GoImports (which supports all of GoFMT’s features, plus normalizes the way import rows are written).

Below we discuss the problem checking that these two automatic tools cannot do.

Comment Sentences

Comment a declaration with a full sentence, usually beginning with the name of the declaration and ending with a period, like this:

// 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) { ...
Copy the code

You can also end comments with an exclamation mark or question mark. The reason for this is that the documents generated using GODoc are very readable.

See the Effective Go documentation for more information.

Contexts

Context.Context is a standard type in the Go language. It often contains information such as security certificates, trace information, expiration time, and cancellation signals that cross API and process boundaries.

An RPC call to the Go program requires the context to be passed explicitly.

Don’t put Context in a structure type, put it in a parameter list, unless your method name cannot be changed (for example, the method signature must match the interface definition in the standard library or third-party library). If placed in a parameter list, usually as the first parameter, it looks like this:

func F(ctx context.Context, /* other arguments */) {}
Copy the code

If you have data to pass, consider using function arguments, receivers, or global variables first. If the data really fits into the Context, use context. Value, do not customize the Context type, and do not extend the Context interface.

Context is immutable, and the same Context can be passed to multiple calls that share information.

Copying

Copying structures from other packages beware of unexpected alias problems. For example, the bytes.buffer type contains a []byte slice. Copies of the Buffer object may point to the same memory as the original object, causing unexpected problems in subsequent calls. Consider the following example:

package main

import (
"bytes"
"fmt"
)

func main() {
	a := bytes.Buffer{}
	a.Write([]byte("hello "))
	b := a
	a.Write([]byte("new world"))

	fmt.Printf("a: %s\n", a.Bytes())
	b.Write([]byte("old"))
	fmt.Printf("a: %s\n", a.Bytes())

	return
}
Copy the code

The output is:

a: hello new world
a: hello old world
Copy the code

In general, do not copy the value of type T if it changes the behavior of internal data.

Declaring Empty Slices

To define an empty slice, we tend to write:

var t []string
Copy the code

Instead of this:

t := []string{}
Copy the code

The former method defines a nil slice value, while the latter defines a non-empty slice of zero length. Both are the same in many cases, such as zero length and zero capacity, but the former is recommended. There are special cases where the two are not equivalent, such as json encoding where the former is encoded as null and the latter as json array [].

When designing interfaces, avoid the subtle programming problems that arise from the differences between the two.

Crypto Rand

Don’t use math/ RAND packages to generate (even one-time) key values. Without random seeds, the generator is perfectly predictable, and even with time.nanoseconds (), the entropy is too low. The alternative is to use the Crypto/RAND package Reader, which converts random text to hexadecimal or Base64 encoding if needed:

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) }Copy the code

Doc Comments

All top-level, exported names need to be documented, as should important internal types and functions. See here for the specification of the document.

Don’t Panic

Common error handling is error and multiple return values. Try not to use Panic.

Error Strings

Error strings do not begin with a capital case (except for certain nouns) and do not end with punctuation marks, as they are often printed in the Error output log. In other words, define an error like this:

fmt.Errorf("something bad") 
Copy the code

Instead of this:

fmt.Errorf("Something bad.")
Copy the code

In this way the user will be much more natural in log printing:

log.Printf("Reading %s: %v.", filename, err)
Copy the code

Examples

When adding a new package, include examples of use: either a runnable example or a test demo of the full call.

Goroutine Lifetimes

When you create a coroutine, know if and when it will exit.

Coroutines that are no longer needed, if not handled properly, can cause some weird problems, such as:

Coroutines blocked by receiving and sending channel messages will leak.
Modifying inputs when the results are not needed leads to unnecessary concurrent data competition;
Sending a message to a closed channel causes Panic.

Keep concurrent code simple and coroutine life cycle clear. If that is not possible, document when and why the coroutine exits.

Handle Errors

Do not discard the error with an “_”; check it to make sure the function call is successful. Handle errors, return them, and even throw Panic if necessary. See here for more details.

Imports

Good package name, in the import of the general will not conflict. Try to avoid renaming imported package names unless there is a name conflict. In the event of a conflict problem, renaming local packages or project-specific package imports is preferred.

Import packages are grouped, separated by empty lines, and the library is placed in the first group:

package main

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

	"appengine/foo"
	"appengine/user"

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

The Goimports tool will help us do this.

Import Dot

Click import to make it easy to write loop-dependent test cases, such as the following code:

package foo_test

import (
	"bar/testutil" // also imports "foo"
	. "foo"
)
Copy the code

The test file bar/testutil imports the foo package, and if the test case needs to import bar/testutil, it cannot be placed under the foo package, otherwise a circular reference will occur. Using point imports at this point, you can pretend that the test case is under the foo package (which is actually under the foo_test package).

In addition to the above, do not use point imports; they seriously affect the readability of your code, and you cannot tell whether a Quux is an identifier of the current package or belongs to an imported package.

In-Band Errors

In C and similar languages, a common practice is to tell the caller that an error has occurred by returning an exception value (such as -1 or a null pointer), which we call in-band Errors, like the following:

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

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

In Go, we recommend returning an additional value to indicate an error rather than an inline error, such as error or a Boolean value, like the following:

// Lookup returns the value for key or ok=false if there is no mapping for key. func Lookup(key string) (value string, ok bool) This prevents the caller from using the result incorrectly: Parse(Lookup(key)) // compile-time error And encourages more robust and readable code: value, ok := Lookup(key) if ! ok { return fmt.Errorf("no value for %q", key) } return Parse(value)Copy the code

This rule applies to both derived and non-derived functions.

Return values like nil, “”, 0, -1 are reasonable if they are the result of a legitimate method call (judged by the fact that the function caller can handle these and other values with the same logic).

Standard libraries like Strings, which return inline error values, simplify string handling at the expense of more effort on the part of the caller.

Indent Error Flow

Indent error handling logic, not regular code. This improves the readability of the code and allows the reader to quickly navigate the logical trunk.

The following code is bad:

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

Change it to the following:

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

If the if statement has initialization logic like this:

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

Move the initialization outside and change it to something like this:

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

Initialisms

Acronyms words or abbreviations (such as “URL” or “NATO”) in a name should have the same case. For example, “URL” can be written as “URL” or “URL”, and in phrases it can be “urlPony” or “urlPony”, but not “URL”. Another example is to write “ServerHTTP” instead of “ServerHTTP”. For names with multiple acronyms, write either “xmlHTTPRequest” or “xmlHTTPRequest”.

For another example of “ID”, write” appID “instead of” appID “for” identifier “abbreviation.

The exception is the automated code generated by the Protocol buffer, which requires different things from people to machines.

Interfaces

In general, Go interfaces should be included in the user’s package, not in the implementation’s package. The implementer only needs to return a concrete type (usually a pointer or structure), which makes it easy to add implementations without the need for extended refactoring.

Don’t define the interface before you use it. Without a real-world usage scenario, we can’t be sure whether an interface has any value, let alone how to design it.

Instead of defining dummy interfaces for implementers to use when testing, define public apis and test with real implementations. For example, the following consumer is the user of the interface and its implementation and test code is as follows:

Package consumer //consumer.go: type Thinger interface {Thing() bool} func Foo(t Thinger) string {... }Copy the code

Package consumer //consumer_test.go: type fakeThinger struct{... } func (t fakeThinger) Thing() bool {... }... If Foo (fakeThinger {... }) == "x" {... }Copy the code

The following interface implementations are not recommended:

// DO NOT DO IT!!! Package producer type Thinger interface {Thing() bool} type defaultThinger struct{... } func (t defaultThinger) Thing() bool {... } func NewThinger() Thinger{return defaultThinger{... }}Copy the code

We should return a concrete type and let the consumer mock out the producer’s implementation:

Package producer Type Thinger struct{... } func (t Thinger) Thing() bool {... } func NewThinger() Thinger{return Thinger{... }}Copy the code

Line Length

There is no standard line length in the Go code, just avoid uncomfortable lengths; Similarly, don’t wrap long code when it’s more readable.

Most unnatural line breaks (during method calls and declarations) can be avoided by choosing a reasonable number of argument lists and appropriate variable names. When a line of code is too long, it’s usually because the names in the code are too long.

In other words, break lines at semantic points rather than just looking at the length of the line. If you find that a line is too long, you can either change the name or adjust the semantics, and that usually solves the problem.

There is no such thing as a “maximum number of characters per line of code,” but there must be a rule that a line of code is too cluttered and requires changing function boundaries.

Mixed Caps

Refer to mixed-caps, the Go language recommends camel naming. One slight difference from other languages is that non-exported constants are named maxLength, not maxLength or MAX_LENGTH.

Named Result Parameters & Naked Returns

Compare the following two function declarations and imagine seeing them in a document:

func (n *Node) Parent1() (node *Node)
func (n *Node) Parent2() (node *Node, err error)
Copy the code

Here’s the second:

func (n *Node) Parent1() *Node
func (n *Node) Parent2() (*Node, error)
Copy the code

Ever feel like the first one is a bit wordy?

Let’s look at another case where a function returns two or three arguments of the same type, or if the meaning of the returned arguments is not clear in the context, it’s useful to give them names.

func (f *Foo) Location() (float64, float64, error)
Copy the code

The top one is not as good as the bottom one:

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

Don’t add a name to a return argument just because you can declare one less variable in the function. Lazy is small, and the code documentation is readable.

Some handy simple functions that return parameters without naming are not a problem. Once the function is larger, explicitly indicate what its return value means. Don’t name the return parameters just for the sake of it; clarity of the code documentation is the first consideration.

Finally, there are some patterns in which the return value needs to be modified in the defer closure, where naming the return value is fine.

Package Comments

Package comments, like other comments displayed through GODoc, must be adjacent to the package declaration, with no empty lines in between, like this:

// Package math provides basic constants and mathematical functions.
package math
Copy the code

Or like this:

/*
Package template implements data-driven templates for generating textual
output such as HTML.
....
*/
package template
Copy the code

For example, if a file is in the seedgen directory, the comments for the main package can be one of the following:

// Binary seedgen ...
package main
or

// Command seedgen ...
package main
or

// Program seedgen ...
package main
or

// The seedgen command ...
package main
or

// The seedgen program ...
package main
or

// Seedgen ..
package main
Copy the code

As a matter of detail, should comments beginning with binary names be capitalized? The answer is yes! Comments are documents, so they follow the grammar of English, which includes capitalizing the first letter of a sentence. As far as the case in the document doesn’t exactly match the actual binary name, that’s as good as it gets. So I recommend this:

// The seedgen command ...
package main
or

// The seedgen program ...
package main
Copy the code

Package Names

For example, if you have a package called Chubby, the variables in the package don’t have to be defined as ChubbyFile. Chubby.chubbyfile), which should be defined as File (chubby.file when used by customers).

To reduce reference collisions, avoid using common words such as util, common, misc, API, and types for package names.

Pass Values

Do not pass pointer arguments to save a few bytes. If a function uses only the *x form of a pointer argument, it should not pass pointer arguments at all. But this recommendation does not apply to parameter passing for big data structures or data structures that may grow.

Receiver Names

The name of the method receiver should be a shorthand, usually with one or two letters of the type prefix. Do not use generic names like me, this, and self. Convention and consistency bring simplicity; if you use C for receivers in one method, don’t use CL in the other.

Receiver Type

One of the things beginners to Go often have trouble choosing is whether to use a value or a pointer for the receiver of a method. A basic rule of thumb is to use Pointers when in doubt, but there are situations where value reception makes more sense and performs better. Here are some useful fine print:

Methods that need to change the internal value of the receiver must use Pointers.
The receiver contains sync.Mutex or similar synchronization fields, which must be pointed to to avoid copying.
The receiver is a large data structure or array, and Pointers are more efficient.
If the receiver is a structure, array, slice, and the internal element has Pointers to mutable elements, then pointer receivers are preferred to provide more explicit semantics.
If it is a map, func, or chan, do not receive with Pointers; Do not use Pointers if it is a slice and the method does not change its value (reslice or reallocate the slice).
If the receiver is a small object or array, which is conceptually a value type (such as time.time) and does not have mutable fields or pointer fields, or is simply a primitive type such as int or string, value receivers are suitable. The value sink reduces garbage memory by attempting to allocate memory on the stack rather than on the heap when passing values through it. But in some cases the compiler may not be that smart, so check this out before you use it.
Finally, if in doubt, use a pointer.

Synchronous Functions

You can use a synchronous function instead of an asynchronous one, which simply means returning the result directly, or calling a callback or completing a channel operation before returning.

Synchronous functions keep coroutines local at call time, making it easier to infer coroutine life cycles, memory leaks, and concurrency races, and easier to test.

If the caller needs concurrent processing, it can simply open a separate coroutine; But for an asynchronous function, it is difficult or sometimes impossible for the caller to change to synchronous.

Useful Test Failures

The failed branch of the test needs to add useful diagnostic information about what the input, actual output, and expected output are, like this:

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 }Copy the code

When you write it, make sure that the actual output and the expected output are not inverted.

If you feel the amount of code is too much, try table-driven testing.

Another way to differentiate between test failure branches is to use different test function names, like this:

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

Either way, the goal is to provide enough diagnostic information to the people who will maintain your code in the future.

Variable Names

Go language variable names are better short than long, especially for local variables with small scope. C, not lineCount; Use I instead of sliceIndex.

As a general rule, the further from declaration to use, the more detailed the variable name. The name of the method’s recipient, one or two characters, is enough, the usual loop subscript, reader reference, one character (I, r) is enough; Less common, global variables should be named more declaratively.