【Golang】Code Review

Posted by 西维蜀黍 on 2021-06-16, Last Modified on 2021-11-13

Summary

  • Use goimports to standardise imports

    • a superset of gofmt which additionally adds (and removes) import lines as necessary.
  • godoc

  • gofmt

  • go lint

Flow

Indent Error Flow

Try to keep the normal code path at a minimal indentation, and indent the error handling, dealing with it first. This improves the readability of the code by permitting visually scanning the normal path quickly. For instance, don’t write:

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

Instead, write:

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

If the if statement has an initialization statement, such as:

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

then this may require moving the short variable declaration to its own line:

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

Ref https://github.com/golang/go/wiki/CodeReviewComments#indent-error-flow

Naming

Initialisms

Words in names that are initialisms or acronyms (e.g. “URL” or “NATO”) have a consistent case. For example, “URL” should appear as “URL” or “url” (as in “urlPony”, or “URLPony”), never as “Url”. As an example: ServeHTTP not ServeHttp. For identifiers with multiple initialized “words”, use for example “xmlHTTPRequest” or “XMLHTTPRequest”.

This rule also applies to “ID” when it is short for “identifier” (which is pretty much all cases when it’s not the “id” as in “ego”, “superego”), so write “appID” instead of “appId”.

Code generated by the protocol buffer compiler is exempt from this rule. Human-written code is held to a higher standard than machine-written code.

Ref https://github.com/golang/go/wiki/CodeReviewComments#initialisms

Mixed Caps

This applies even when it breaks conventions in other languages. For example an unexported constant is maxLength not MaxLength or MAX_LENGTH or underscores to write multiword names.

Ref

Package Names

Ref

Avoid repetition

All references to names in your package will be done using the package name, so you can omit that name from the identifiers. For example, if you are in package chubby, you don’t need type ChubbyFile, which clients will write as chubby.ChubbyFile. Instead, name the type File, which clients will write as chubby.File.

Since client code uses the package name as a prefix when referring to the package contents, the names for those contents need not repeat the package name. The HTTP server provided by the http package is called Server, not HTTPServer. Client code refers to this type as http.Server, so there is no ambiguity.

Avoid meaningless package names like util, common, misc, api, types, and interfaces.

Keep Short

Good package names are short and clear. They are lower case, with no under_scores or mixedCaps. They are often simple nouns, such as:

  • time (provides functionality for measuring and displaying time)
  • list (implements a doubly linked list)
  • http (provides HTTP client and server implementations)

The style of names typical of another language might not be idiomatic in a Go program. Here are two examples of names that might be good style in other languages but do not fit well in Go:

  • computeServiceClient
  • priority_queue

Abbreviate judiciously

Package names may be abbreviated when the abbreviation is familiar to the programmer. Widely-used packages often have compressed names:

  • strconv (string conversion)
  • syscall (system call)
  • fmt (formatted I/O)

On the other hand, if abbreviating a package name makes it ambiguous or unclear, don’t do it.

Ref

Interface Names

By convention, one-method interfaces are named by the method name plus an -er suffix or similar modification to construct an agent noun: Reader, Writer, Formatter, CloseNotifier etc.

There are a number of such names and it’s productive to honor them and the function names they capture. Read, Write, Close, Flush, String and so on have canonical signatures and meanings. To avoid confusion, don’t give your method one of those names unless it has the same signature and meaning. Conversely, if your type implements a method with the same meaning as a method on a well-known type, give it the same name and signature; call your string-converter method String not ToString.

Ref https://golang.org/doc/effective_go#interface-names

Function Names

Getters/Setters

Go doesn’t provide automatic support for getters and setters. There’s nothing wrong with providing getters and setters yourself, and it’s often appropriate to do so, but it’s neither idiomatic nor necessary to put Get into the getter’s name.

If you have a field called owner (lower case, unexported), the getter method should be called Owner (upper case, exported), not GetOwner. The use of upper-case names for export provides the hook to discriminate the field from the method.

A setter function, if needed, will likely be called SetOwner. Both names read well in practice:

owner := obj.Owner()
if owner != user {
    obj.SetOwner(user)
}

Ref https://golang.org/doc/effective_go#Getters

Receiver Names

The name of a method’s receiver should be a reflection of its identity; often a one or two letter abbreviation of its type suffices (such as “c” or “cl” for “Client”).

Don’t use generic names such as “me”, “this” or “self”, identifiers typical of object-oriented languages that gives the method a special meaning. In Go, the receiver of a method is just another parameter and therefore, should be named accordingly. The name need not be as descriptive as that of a method argument, as its role is obvious and serves no documentary purpose. It can be very short as it will appear on almost every line of every method of the type; familiarity admits brevity. Be consistent, too: if you call the receiver “c” in one method, don’t call it “cl” in another.

Ref https://github.com/golang/go/wiki/CodeReviewComments#receiver-names

Variable Names

Variable names in Go should be short rather than long. This is especially true for local variables with limited scope. Prefer c to lineCount. Prefer i to sliceIndex.

The basic rule: the further from its declaration that a name is used, the more descriptive the name must be. For a method receiver, one or two letters is sufficient. Common variables such as loop indices and readers can be a single letter (i, r). More unusual things and global variables need more descriptive names.

Ref https://github.com/golang/go/wiki/CodeReviewComments#variable-names

Slice

Declaring Empty Slices

When declaring an empty slice, prefer

var t []string

over

t := []string{}

The former declares a nil slice value, while the latter is non-nil but zero-length. They are functionally equivalent—their len and cap are both zero—but the nil slice is the preferred style.

Note that there are limited circumstances where a non-nil but zero-length slice is preferred, such as when encoding JSON objects (a nil slice encodes to null, while []string{} encodes to the JSON array []).

When designing interfaces, avoid making a distinction between a nil slice and a non-nil, zero-length slice, as this can lead to subtle programming errors.

Error

Error Strings

Error strings should not be capitalized (unless beginning with proper nouns or acronyms) or end with punctuation, since they are usually printed following other context. That is, use fmt.Errorf("something bad") not fmt.Errorf("Something bad"), so that log.Printf("Reading %s: %v", filename, err) formats without a spurious capital letter mid-message. This does not apply to logging, which is implicitly line-oriented and not combined inside other messages.

Handle Errors

Do not discard errors using _ variables. If a function returns an error, check it to make sure the function succeeded. Handle the error, return it, or, in truly exceptional situations, panic.

Example

Library routines must often return some sort of error indication to the caller. As mentioned earlier, Go’s multivalue return makes it easy to return a detailed error description alongside the normal return value. It is good style to use this feature to provide detailed error information. For example, as we’ll see, os.Open doesn’t just return a nil pointer on failure, it also returns an error value that describes what went wrong.

By convention, errors have type error, a simple built-in interface.

type error interface {
    Error() string
}

A library writer is free to implement this interface with a richer model under the covers, making it possible not only to see the error but also to provide some context. As mentioned, alongside the usual *os.File return value, os.Open also returns an error value. If the file is opened successfully, the error will be nil, but when there is a problem, it will hold an os.PathError:

// PathError records an error and the operation and
// file path that caused it.
type PathError struct {
    Op string    // "open", "unlink", etc.
    Path string  // The associated file.
    Err error    // Returned by the system call.
}
		
func (e *PathError) Error() string {
    return e.Op + " " + e.Path + ": " + e.Err.Error()
}

PathError’s Error generates a string like this:

open /etc/passwx: no such file or directory

Such an error, which includes the problematic file name, the operation, and the operating system error it triggered, is useful even if printed far from the call that caused it; it is much more informative than the plain “no such file or directory”.

From https://golang.org/doc/effective_go#errors

Don’t Panic

Don’t use panic for normal error handling (except for an intended situation). Use error and multiple return values.

Ref https://github.com/golang/go/wiki/CodeReviewComments#dont-panic

In-Band Errors

In C and similar languages, it’s common for functions to return values like -1 or null to signal errors or missing results:

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

Go’s support for multiple return values provides a better solution.

Instead of requiring clients to check for an in-band error value, a function should return an additional value to indicate whether its other return values are valid.

This return value may be an error, or a boolean when no explanation is needed. It should be the final return value.

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

This rule applies to exported functions but is also useful for unexported functions.

Return values like nil, “”, 0, and -1 are fine when they are valid results for a function, that is, when the caller need not handle them differently from other values.

Some standard library functions, like those in package “strings”, return in-band error values. This greatly simplifies string-manipulation code at the cost of requiring more diligence from the programmer. In general, Go code should return additional values for errors.

Ref https://github.com/golang/go/wiki/CodeReviewComments#in-band-errors

Package

Import

Avoid Renaming Imports

Avoid renaming imports except to avoid a name collision; good package names should not require renaming. In the event of collision, prefer to rename the most local or project-specific import.

Imports are organized in groups, with blank lines between them. The standard library packages are always in the first group.

package main

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

	"appengine/foo"
	"appengine/user"

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

Ref https://github.com/golang/go/wiki/CodeReviewComments#imports

Import Blank

package main

import (
    "fmt"
    _ "foo/bar/baz"
)

func main() {
    fmt.Println(baz.Hello(), baz.World()) // 错误 _ 并没有导入包 只是引入并执行包模块的 init  方法
}

_ 是包引用操作,只会执行包下各模块中的 init 方法,并不会真正的导入包,所以不可以调用包内的其他方法。


Packages that are imported only for their side effects (using the syntax import _ "pkg") should only be imported in the main package of a program, or in tests that require them.

Ref https://github.com/golang/go/wiki/CodeReviewComments#import-blank

Import Dot

这里的. 符号表示:在调用包 lib 的成员时,不需要再声明包名。

比如,Println() 是 fmt 包中的方法,在一般的情况下,我们都通过 fmt.Println() 来调用。而使用 . "fmt" 后,直接可以调 Println()

package main
import {
	. "github.com/libragen/felix/lib"
	. "fmt"
}
func main() {
	SayHello()
	Println("test")
}

The import . form can be useful in tests that, due to circular dependencies, cannot be made part of the package being tested:

package foo_test

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

In this case, the test file cannot be in package foo because it uses bar/testutil, which imports foo. So we use the ‘import .’ form to let the file pretend to be part of package foo even though it is not. Except for this one case, do not use import . in your programs. It makes the programs much harder to read because it is unclear whether a name like Quux is a top-level identifier in the current package or in an imported package.

Ref https://github.com/golang/go/wiki/CodeReviewComments#import-dot

Interface

Misc

Line Length

There is no rigid line length limit in Go code, but avoid uncomfortably long lines. Similarly, don’t add line breaks to keep lines short when they are more readable long–for example, if they are repetitive.

Most of the time when people wrap lines “unnaturally” (in the middle of function calls or function declarations, more or less, say, though some exceptions are around), the wrapping would be unnecessary if they had a reasonable number of parameters and reasonably short variable names. Long lines seem to go with long names, and getting rid of the long names helps a lot.

In other words, break lines because of the semantics of what you’re writing (as a general rule) and not because of the length of the line. If you find that this produces lines that are too long, then change the names or the semantics and you’ll probably get a good result.

This is, actually, exactly the same advice about how long a function should be. There’s no rule “never have a function more than N lines long”, but there is definitely such a thing as too long of a function, and of too repetitive tiny functions, and the solution is to change where the function boundaries are, not to start counting lines.

Ref https://github.com/golang/go/wiki/CodeReviewComments#line-length

Function

Named Result Parameters

Consider what it will look like in godoc. Named result parameters like:

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

will be repetitive in godoc; better to use:

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

On the other hand, if a function returns two or three parameters of the same type, or if the meaning of a result isn’t clear from context, adding names may be useful in some contexts. Don’t name result parameters just to avoid declaring a var inside the function; that trades off a minor implementation brevity at the cost of unnecessary API verbosity.

func (f *Foo) Location() (float64, float64, error)

is less clear than:

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

Naked returns are okay if the function is a handful of lines. Once it’s a medium sized function, be explicit with your return values. Corollary: it’s not worth it to name result parameters just because it enables you to use naked returns. Clarity of docs is always more important than saving a line or two in your function.

Finally, in some cases you need to name a result parameter in order to change it in a deferred closure. That is always OK.

Ref https://github.com/golang/go/wiki/CodeReviewComments#named-result-parameters

Receiver Type

Choosing whether to use a value or pointer receiver on methods can be difficult, especially to new Go programmers.

**If in doubt, use a pointer, but there are times when a value receiver makes sense, usually for reasons of efficiency, such as for small unchanging structs or values of basic type. **

Some useful guidelines:

  • If the receiver is a map, func or chan, don’t use a pointer to them. If the receiver is a slice and the method doesn’t reslice or reallocate the slice, don’t use a pointer to it.
  • If the method needs to mutate the receiver, the receiver must be a pointer.
  • If the receiver is a struct that contains a sync.Mutex or similar synchronizing field, the receiver must be a pointer to avoid copying.
  • If the receiver is a large struct or array, a pointer receiver is more efficient. How large is large? Assume it’s equivalent to passing all its elements as arguments to the method. If that feels too large, it’s also too large for the receiver.
  • Can function or methods, either concurrently or when called from this method, be mutating the receiver? A value type creates a copy of the receiver when the method is invoked, so outside updates will not be applied to this receiver. If changes must be visible in the original receiver, the receiver must be a pointer.
  • If the receiver is a struct, array or slice and any of its elements is a pointer to something that might be mutating, prefer a pointer receiver, as it will make the intention more clear to the reader.
  • If the receiver is a small array or struct that is naturally a value type (for instance, something like the time.Time type), with no mutable fields and no pointers, or is just a simple basic type such as int or string, a value receiver makes sense. A value receiver can reduce the amount of garbage that can be generated; if a value is passed to a value method, an on-stack copy can be used instead of allocating on the heap. (The compiler tries to be smart about avoiding this allocation, but it can’t always succeed.) Don’t choose a value receiver type for this reason without profiling first.
  • Don’t mix receiver types. Choose either pointers or struct types for all available methods.
  • Finally, when in doubt, use a pointer receiver.

Ref https://github.com/golang/go/wiki/CodeReviewComments#receiver-type

Comments

Package Comments

Package comments, like all comments to be presented by godoc, must appear adjacent to the package clause, with no blank line.

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

For “package main” comments, other styles of comment are fine after the binary name (and it may be capitalized if it comes first), For example, for a package main in the directory seedgen you could write:

// Binary seedgen ...
package main

These are examples, and sensible variants of these are acceptable.

Note that starting the sentence with a lower-case word is not among the acceptable options for package comments, as these are publicly-visible and should be written in proper English, including capitalizing the first word of the sentence. When the binary name is the first word, capitalizing it is required even though it does not strictly match the spelling of the command-line invocation.

Ref

Example

Every package should have a package comment, a block comment preceding the package clause. For multi-file packages, the package comment only needs to be present in one file, and any one will do. The package comment should introduce the package and provide information relevant to the package as a whole. It will appear first on the godoc page and should set up the detailed documentation that follows.

/*
Package regexp implements a simple library for regular expressions.

The syntax of the regular expressions accepted is:

    regexp:
        concatenation { '|' concatenation }
    concatenation:
        { closure }
    closure:
        term [ '*' | '+' | '?' ]
    term:
        '^'
        '$'
        '.'
        character
        '[' [ '^' ] character-ranges ']'
        '(' regexp ')'
*/
package regexp

If the package is simple, the package comment can be brief.

// Package path implements utility routines for
// manipulating slash-separated filename paths.

Function Comments

Complete Sentences

Doc comments work best as complete sentences, which allow a wide variety of automated presentations. The first sentence should be a one-sentence summary that starts with the name being declared.

// Compile parses a regular expression and returns, if successful,
// a Regexp that can be used to match against text.
func Compile(str string) (*Regexp, error) {

If every doc comment begins with the name of the item it describes, you can use the doc subcommand of the go tool and run the output through grep. Imagine you couldn’t remember the name “Compile” but were looking for the parsing function for regular expressions, so you ran the command,

$ go doc -all regexp | grep -i parse

Proper Capitalization in Comments

Nearly every top-level type, const, var and func should have a comment. A comment for bar should be in the form “bar floats on high o’er vales and hills.”. The first letter of bar should not be capitalized unless it’s capitalized in the code.

// enterOrbit causes Superman to fly into low Earth orbit, a position
// that presents several possibilities for planet salvation.
func enterOrbit() os.Error {
  ...
}

All text that you indent inside a comment, godoc will render as a pre-formatted block. This facilitates code samples.

// fight can be used on any enemy and returns whether Superman won.
//
// Examples:
//
//  fight("a random potato")
//  fight(LexLuthor{})
//
func fight(enemy interface{}) bool {
  ...
}

Ref https://github.com/golang/go/wiki/Comments

Declaration

Const Variables

Go’s declaration syntax allows grouping of declarations. A single doc comment can introduce a group of related constants or variables. Since the whole declaration is presented, such a comment can often be perfunctory.

// Error codes returned by failures to parse an expression.
var (
    ErrInternal      = errors.New("regexp: internal error")
    ErrUnmatchedLpar = errors.New("regexp: unmatched '('")
    ErrUnmatchedRpar = errors.New("regexp: unmatched ')'")
    ...
)

Grouping can also indicate relationships between items, such as the fact that a set of variables is protected by a mutex.

var (
    countLock   sync.Mutex
    inputCount  uint32
    outputCount uint32
    errorCount  uint32
)

Ref https://golang.org/doc/effective_go#commentary

Reference