golang/go CodeReviewComments 日本語翻訳

content.md

Go Code Review Comments

原文

go fmt

あなたのコードに gofmt を走らせると、自動的に機械的に直すことのできるスタイルの大部分を修正してくれます。 世にあるGolang コードのほとんどすべてが gofmt を使っています。 この文章の残りは機械的に直すことのできないポイントについて解説します。

代わりに goimports を使う手段もあります。 gofmt に加えて必要に応じてimport内に空行をつけたり消したりする機能があります。

Comment Sentences

http://golang.org/doc/effective_go.html#commentary を読みましょう。 func や struct などのドキュメントのためのコメントは多少冗長であっても完全な文章でなくてはいけません。 このアプローチは godoc ドキュメントにするときにより効果を発揮します。 コメントは以下の例のように対象の名前で始まって、ピリオドで終わらなければいけません。

// A 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) { ...

Declaring Empty Slices

スライスの宣言の時は

var t []string

のほうが

t := []string{}

よりも良いです。 前者はもしスライスになにも追加されなくても、余計なメモリを消費しません。

Doc Comments

全ての最上位にあったり外部に公開されているものは、docコメントがなければいけません。 外部に公開されていない重要な関数や type 宣言にも同様です。 http://golang.org/doc/effective_go.html#commentary を読むと、よりコメントの規則についての情報を得ることができます。

Don't Panic

http://golang.org/doc/effective_go.html#errors を読みましょう。 通常のエラーハンドリングで panic を使うのを辞めましょう。 なるべく error 型を含んだ複数の値を返すようにしましょう。

Error Strings

エラー文字列は頭文字を大文字にしたり、句読点で終わってはいけません。 なぜならこの文字列は他のコンテキストで使われるケースがあるからです。 つまり、log.Print("Reading %s: %v", filename, err) が途中で大文字を出力することがないように fmt.Errorf("Something bad") ではなく、 fmt.Errorf("something bad") を使いましょう。 これはロギングのような暗黙的な行指向や、他のメッセージと組み合わせないものには適用されません。

Handle Errors

http://golang.org/doc/effective_go.html#errors を読みましょう。 _ でエラーを無視してはいけません。 もし関数がエラーを返してきたら、関数がちゃんと成功したか確認するためにチェックしてください。 エラーをハンドリングして返すか、本当に例外的な場合のみ panic しましょう。

Imports

import を　空行で幾つかのグループに分けましょう。 標準パッケージは最初にグループにしましょう。 goimports がこれは自動で行ってくれます。

Import Dot

. を使ったインポートは循環参照によってパッケージの一部がテストできない時などに役立ちます。

package foo_test

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

このケースでは、テストファイルはfooをインポートしているbar/testutilを使っていて、循環参照になってしまうのでpackage fooに置くことができません。 なので import . を使ってpackage fooの一部のように見せかけます。 このケースを除いて、import . をあなたのプログラムで使うべきではありません。 Quuxのような名前が今のパッケージの識別子なのかインポートされたものなのかが非常にわかりにくくなって、可読性が極端に落ちるからです。

Indent Error Flow

初めにコード、特にエラーハンドリングの分岐を最小になるように努めましょう。 普通の流れは簡単に追うことができるので、こうすることで可読性が上がります。

例えば、こうするのではなく、

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

こう書くべきです。

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

もし初期化のコードでこのようなコードがあるなら

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" は "URL" か "url" でなければならず、"Url" ではいけません。 "ServeHttp" ではなく "ServeHTTP" となります。

"identifier" の省略である "ID" にもこのルールは適用され、 "appId" ではなく "appID" となります。

プロトコルバッファコンパイラによって生成されたコードはこのルールを免除します。 人が書いたコードには、機械が書いたコードよりも厳しい基準を要求します。

Line Length

Golangでは1行の長さを決めてはいませんが、長過ぎないようにしてください。 同じように繰り返しが多い時など、1行を短く保ちたいがために無理に改行を入れる必要はないでしょう。

コメントは通常80文字になる前に改行されるべきです。これはルールだからではなく、何百列もの表示するエディタではそうしたほうがより見やすいからです。 人間は大きなエディタでないと開けないような幅広のテキストではなく、新聞のコラムのような横幅の狭いテキストを読むほうが適しています。 とにかくどちらにせよgodocはいい感じに表示しなければいけません。

Mixed Caps

http://golang.org/doc/effective_go.html#mixed-caps を読んでください。 これは他の言語の取り決めとは異なります。 例えば、公開しない定数は MaxLength や MAX_LENGTH ではなく maxLength となります。

Named Result Parameters

このような名前付き戻り値がGodocではどのように見えるか考えてみましょう

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

なにかぎこちない感じになります。こちらのほうがよいでしょう。

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

一方で関数が同じ型の返り値を複数返したり、結果が文脈からはっきりとわかりにくい場合、名前付き戻り値は有効です。 この例よりも

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)

ただの return は数行の関数なら大丈夫ですが、ある程度の大きさの関数になると、返す変数を明確にしたほうがよいでしょう。 当然ですが、ただの return が使えるというだけでは名前付き戻り値を使う理由にはなりません。 明快なドキュメントを書くことはたかが１〜２行を節約するよりも何倍も価値があります。

最後に、幾つかのケースでは遅延クロージャで値を変更するために名前付き戻り値を指定する必要があります。 その場合は大丈夫です。

Naked Returns

Named Result Parameters と同じ

Package Comments

Godocで読める全てのコメントと同じようにパッケージのコメントはパッケージ節の前に空行無しで書かなければいけません。

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

http://golang.org/doc/effective_go.html#commentary を読むとより詳細なコメントに関するアドバイスが有ります。

Package Names

パッケージから公開されている全ての識別子への参照はパッケージ名を通して行われるので、識別子にパッケージ名を使っている場合は外すべきです。 例えば、chubbyというパッケージの中ではChubbyFileという名前を作っては、呼び出すときにchubby.ChubbyFileとなってしまうので、避けましょう。 その代わりにFileという名前にして、使う側がchubby.Fileとできるのでよりよいでしょう。 http://golang.org/doc/effective_go.html#package-names　にはより多くの情報があります。

Pass Values

たかだか数バイトを節約するために引数にポインタを指定するのは辞めましょう。 もし関数が全体を通して引数 xを *x として呼び出しているなら、引数をポインタにするべきではありません。 この一般的なインスタンスは文字列のポインタ(*string)や、インターフェースのポインタ(*io.Reader)を渡すことも含みます。 どちらのケースでも、変数自身は固定されていて、直接値が渡されます。 このアドバイスは、大きなStructや今後大きくなりそうなものには適用しません。

Receiver Names

レシーバの名前はそれがなんであるかを適切に表したものでなければいけません。 大抵の場合その型1〜2文字の略称で足ります。 (Clientであれば　c や cl　のように) me や this, selfのように関数ではなく、メソッドに重点を置いたオブジェクト指向の典型的な名前を使うのは辞めましょう。 レシーバ名はその役割が明らかで、ドキュメントとしての目的もないので、引数名ほど説明的である必要がありません。 そのメソッドのあらゆる行に登場するので、できるだけ短いほうがよいでしょう。慣れてくればとても簡素でよく思えてきます。 レシーバ名は統一してください。あるところで　client を c としたなら、他のところで　cl としてはいけません。

Receiver Type

メソッドで値を使うか、ポインタを受け取るかを選択することは常に難しいものです。特に新米Gopherには難しいでしょう。 疑わしい場合はポインタを受け取りますが、小さい構造体や、プリミティブな型の場合は、値を受け取るだけのほうが効率的な場合があります。 便利なガイドラインを下に示します。

• レシーバが map、func、チャネル　ならポインタを使うべきではありません。
• レシーバがスライスで、メソッドがスライスを作りなおさない場合は、ポインタを使うべきではありません。
• メソッドが値を変更する必要がある場合、レシーバはポインタでなければいけません。
• レシーバが sync.Mutex か、似たような同期するフィールドを持つ構造体なら、レシーバはポインタでなければいけません。
• レシーバが大きな構造体や配列なら、ポインタはとても効果的です。では大きいとはどれくらいなのでしょう？もし構造体の全ての値を引数に渡すと仮定してください。多すぎると感じたなら、それはポインタにしても良いくらいの大きさです。
• 関数が同時に実行されたりメソッドが呼び出された時に、レシーバの値を変更するでしょうか？値渡しではメソッドが実行されるときにレシーバのコピーを生成します。なのでメソッドの外ではレシーバへの変更が適用されません。変更がオリジナルのレシーバに適用される必要があるなら、レシーバはポインタです。
• レシーバが、値を変更されるかも知れない構造体、配列、スライス、その他の要素出会った場合、レシーバをポインタにしたほうが、読み手にとってよりわかりやすいでしょう。
• レシーバが小さく、本来値型であったり(例えば time.Time　のようなもの)　、変更するフィールドやポインタがない構造体や配列、あるいは int や string のようなシンプルな型の場合は、レシーバが値であるほうが良い場合があります。値のみのレシーバは生成されるゴミの量を減らすことができます。もし値がメソッドに渡されると、ヒープ領域にメモリを確保する代わりに、スタックメモリのコピーが走ります。(コンパイラはこのヒープメモリ確保を避けようとしますが、常には上手く行きません)　プロファイラで確認する前にこの理由で値レシーバにするのは辞めましょう。
• これらの理由に当てはまらず、まだ迷っている場合はレシーバをポインタにしましょう。

Useful Test Failures

テストが失敗した時には、以下の事柄を伝える有効なメッセージを出力しましょう。

• なにが悪かったのか
• どんな入力があったか
• 実際にどんな値が来たのか
• どんな値が来ることを期待していたのか

AssertFoo ヘルパーの束をを書くことは魅力的ですが、あなたのヘルパーはもっと役に立つメッセージを出力できるはずです。 あなたの失敗したテストをデバッグする人はあなたや、あなたのチームではないことを前提にしています。 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 != order の順で並んでいて、メッセージでも同じ順番で出力していることに注目してください。 幾つかのテストフレームワークでは 0 != x, "expected 0, got x" のように、この順を逆にして書くことを推奨していますが、Golangではそうではありません。

もしこのやり方で大量にタイプしなければいけないようなら、Table-Driven-Testが欲しくなるかもしれません。

もう1つのテスト失敗時の曖昧さをなくすためのテクニックとして、入力ごとに TestFoo をラップした異なるテスト関数を作る小方法があります。この場合関数の名前と共に失敗します。

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

いずれにせよ、将来あなたのコードをデバッグする人に有効なメッセージを届ける責任はあなたにあります。

Variable Names

Golangでの変数名はより短くあるべきです。 特に限定されたスコープで使われるローカル変数は短くあるべきです。 lineCount　よりも c ですし、 lineIndex よりも i です。

基本的なルール

• 宣言された箇所よりも離れた場所でその変数が使われるなら、より説明的な名前にするべきです。
• メソッドレシーバは1〜2文字で十分です
• ループの添字や読み込みのリーダーのような一般的な変数は(i や r のように)1文字でよいでしょう。
• より珍しい物や、パッケージ外で使われるものの名前はより説明的にしましょう。