この記事を書いた目的
以前に社内で先輩に自分が書いたコードを丁寧にレビューしてもらったことがあります。
この時に学んだ教訓を忘れないようにしっかり記事にアウトプットしてまとめようと思ったのが目的になります。
基礎的な内容だったり、当たり前と思ってる人からするとなんてことはない内容かもしれませんが、意外とこういったコードをより良くするための記事や本って少ない気がします,,,
(実務で得たリファクタの知見)
私自身も「強い人ってこういった観点でコードをレビューしているんだ」って
とても学びになりました。
ぜひ誰かの参考になれば幸いです。
書いてたら長くなったのでコードを書くときにより読みやすいコードを書くための「考え方編」とGolangで書くときにより読みやすく書くための「golang編」に分けました。
この記事では「考え方編」を書いてます。
今回取り扱うサンプルコードについて
今回以下のコードを改善していきます。
取り扱う内容として、レビューで指摘もらった箇所を解説していきます。
ちなみにこのソースコードが何をしているかというと
hogeAPIから受け取ったJSON構造体によってリクエストが成功したのか失敗なのかをLambdaを用いて通知するシステムになります。
package main
import (
"bytes"
"context"
"encoding/json"
"fmt"
"io"
"net/http"
"time"
"github.com/aws/aws-lambda-go/lambda"
)
type CloudWatchInput struct {
MessageType string `json:"messageType"`
Owner string `json:"owner"`
LogGroup string `json:"logGroup"`
LogStream string `json:"logStream"`
SubscriptionFilters []string `json:"subscriptionFilters"`
LogEvents []struct {
ID string `json:"id"`
Timestamp int64 `json:"timestamp"`
Message string `json:"message"`
} `json:"logEvents"`
}
type hogeAPIRequestParameter struct {
Dt string `json:"Dt"`
Name string `json:"Name"`
SrcCd string `json:"srcCd"`
SrcContent string `json:"srcContent"`
ID string `json:"ID"`
}
type hogeAPIResponse struct {
ID string `json:"id"`
Code int `json:"code"`
Description string `json:"description"`
}
func hogeApiRequests(ctx context.Context, input *CloudWatchInput) ([]*hogeAPIResponse, error) {
if input == nil {
return nil, fmt.Errorf("received nil event")
}
if len(input.LogEvents) == 0 {
return nil, fmt.Errorf("logEvents array is empty")
}
var results []*hogeAPIResponse
for _, logEvent := range input.LogEvents {
var message map[string]interface{}
if err := json.Unmarshal([]byte(logEvent.Message), &message); err != nil {
return nil, fmt.Errorf("failed to unmarshal log event message: %v", err)
}
timeStr, ok := message["time"].(string)
if !ok {
return nil, fmt.Errorf("failed to get time from log event message")
}
parsedTime, err := time.Parse(time.RFC3339, timeStr)
if err != nil {
return nil, fmt.Errorf("failed to parse time: %v", err)
}
occurredDt := parsedTime.Format("20060102150405")
hogeapiRequestParameter := &hogeAPIRequestParameter{
Dt: occurredDt,
Name: message["uri"].(string),
SrcCd: "",
SrcContent: logEvent.Message,
ID: message["request_id"].(string),
}
hogeRequestJSON, err := json.Marshal(hogeapiRequestParameter)
if err != nil {
return nil, fmt.Errorf("failed to marshal request parameter: %v", err)
}
hogeResp, err := http.Post("https://hogehoge/hogehoge", "application/json", bytes.NewBuffer(hogeRequestJSON))
if err != nil {
return nil, fmt.Errorf("failed to send request: %v", err)
}
defer hogeResp.Body.Close()
var apiResponse hogeAPIResponse
hogeRespBody, err := io.ReadAll(hogeResp.Body)
if err != nil {
return nil, fmt.Errorf("failed to read response body: %v", err)
}
apiResponse.ID = message["request_id"].(string)
apiResponse.Code = hogeResp.StatusCode
apiResponse.Description = string(hogeRespBody)
results = append(results, &apiResponse)
}
return results, nil
}
func main() {
lambda.Start(func(ctx context.Context, input *CloudWatchInput) ([]*hogeAPIResponse, error) {
hogeApiResponses, err := hogeApiRequests(ctx, input)
if err != nil {
return []*hogeAPIResponse{{Code: 500, Description: fmt.Sprintf("ERROR: %v", err)}}, nil
}
allSuccess := true
var errorResponses []*hogeAPIResponse
for _, response := range hogeApiResponses {
if response.Code != 200 {
allSuccess = false
errorResponses = append(errorResponses, response)
}
}
if allSuccess {
return []*hogeAPIResponse{{Code: 200, Description: "CREATED: 送信が成功しました"}}, nil
} else {
return errorResponses, nil
}
})
}
似たようなシステムで上記記事も書いてます
リファクタした点
コードを書くときの基本的な考え方として
「他の人が読んでも違和感なく、読みやすいコードを書く」
自分が実装したコードがなぜそう実装したのか説明できるように意識すると良いと教わりました。
他にも以下を意識すると良いと。
- 可読性を求めてコードを書く→正解を探して書くのではない
- 他の人が読んでも違和感、混乱させないコード
- 何かルール作って愚直に書くと読みづらくなることもある
- 2週間後の自分が理解できるか
- そこに懸念があると改善の余地あり
具体的にどういったことになるのかというのを説明していきます。
※我もの顔で解説しますが、先輩エンジニアに指摘された内容(受け売り)であることをご了承ください笑
変数名は名詞、関数名は動詞
func hogeApiRequests(ctx context.Context, input *CloudWatchInput) ([]*hogeAPIResponse, error) {
この関数名は「[]*hogeApiRequestのようなインスタンスに思えて」分かりづらいと指摘をもらいました。
「変数(構造体)名を定義するのに関数likeな命名は読みづらい,,,,操作(関数)的な印象を受けますよ」と。
逆も然りです。(関数名で変数likeな命名だと変数的な印象を受ける)
func handleCloudWatchLog
上記に修正を加えました。
ポイントは(基本的に)
- 変数名は名詞
- 関数名は動詞(操作的な命名)
を意識して定義する
可読性の下がる省略をしない
変数名を定義している時にreqやiなど変数名を省略して書くことってあると思います。
今回のコードだと以下の部分になります
type hogeAPIRequestParameter struct {
Dt string `json:"Dt"`// DateをDtと省略している
Name string `json:"Name"`
SrcCd string `json:"srcCd"`//CodeをCdと省略している
SrcContent string `json:"srcContent"`
ID string `json:"ID"`
}
今回の場合、構造体のfield名は外部APIの仕様で定義されていたものをそのまま持ってきたのですが、その命名をそのまま書き写せば、自分の技術的負債になります。
OccurredDtやMntCdという箇所のDtやCdというのが何の省略なのか推測しづらいです。他にも
-
× res 〇result→resはresponseとも推測できるのでわかりづらい -
× tbl 〇table→略してもそんなに文字数変わらないし、略したら推測しづらい
などなど、こういった省略は読み手を混乱させる可能性があるのでよくないです。
逆に省略しても問題ない時もあります。
例えば
-
for文の時のindexのi
などは読み手もすぐに理解しやすいため、
省略しても問題ないです。
省略したらなぜよろしくないのか
例えば3000行も書かれているコードでtと省略されている変数があったとします。
そうすると、なんのtなのかとても追いづらいと思いませんか？
省略することでこう言ったリスクが生じます。
結局省略していいのかの判断基準として実態が掴めるのか,掴めないのかが重要になってくると教わりました。
その変数、関数が何を目的としているかの実態が掴めるように命名していくこと
わかりづらい変数名(実態が掴みづらい)
変数を定義するときに意識することは他の人が読んだ時に読み手に伝わりやすいコードを書くことです。
そのためにもシンプルで伝わりやすい変数名にすることが大事です。
type hogeAPIResponse struct {
ID string `json:"id"`
Code int `json:"code"`
Description string `json:"description"`
}
今回のコードであればこちらです。(変数ではなく構造体ですが,,,)
この構造体はhogeAPIからのレスポンスのための構造体ではなく、この型が表現したいのは、外部APIから受け取った値を処理した結果です。なので本来の意図と書いてる命名に違和感があります。
よって
type hogeAPIResults struct {
ID string `json:"id"`
Code int `json:"code"`
Description string `json:"description"`
}
と修正しました。
意図として「API送信を含めた処理の結果」が伝わるように命名したというのがあります。
ポイントは
変数名から目的と型が一目で分かること
が大切になるのかなと思います。
関連記事
記事を書きながら調べてくうちに言いたいことが簡潔に書いてある記事を見つけたので紹介しておきます。
publicとprivateの使い分け
type CloudWatchInput struct {//他のpackageで使わないのにpublicになっている
MessageType string `json:"messageType"`
Owner string `json:"owner"`
LogGroup string `json:"logGroup"`
LogStream string `json:"logStream"`
SubscriptionFilters []string `json:"subscriptionFilters"`
LogEvents []struct {
ID string `json:"id"`
Timestamp int64 `json:"timestamp"`
Message string `json:"message"`
} `json:"logEvents"`
}
運用保守が中心のpjや実装しかやってこない(過去のワイ)と正直あまりここら辺を意識することって少ないのかなって思うのですが、publicとprivate使い分けはすごく重要になります。
前提条件として今回書いたコードは他packageで参照することはない方針で実装しています。つまり他で使わない関数や構造体、変数をpublicで定義してしまうとパッケージ設計、APIの安定性、セキュリティの観点からリスクを生じます。
外部packageからアクセスする必要のない関数、構造体なのかどうかはしっかり意識して実装していきたいです。
インスタンスの宣言は直前に
hogeApiRequests関数の最初の方にvar results []*hogeAPIResponseって定義しているのに使っているのは最後の方です。
func hogeApiRequests(ctx context.Context, input *CloudWatchInput) ([]*hogeAPIResponse, error) {
var results []*hogeAPIResponse
,,,省略 //hogeApiRequestsの最後らへんに
results = append(results, &apiResponse)//定義して数十行後に使っている
}
return results, nil
}
こうしてしまうと読んでいくうちに「あれ、resultsって何だっけ？」と読みづらくなり、可読性が下がるのでやめましょう。
変数の定義は使用する直前に書く
なるべく変数は定義しない方針で
func hogeApiRequests(ctx context.Context, input *CloudWatchInput) ([]*hogeAPIResponse, error) {
,,,
var apiResponse hogeAPIResponse
hogeRespBody, err := io.ReadAll(hogeResp.Body)
if err != nil {
return nil, fmt.Errorf("failed to read response body: %v", err)
}
//空のインスタンスをセットしている↓
apiResponse.ID = message["request_id"].(string)
apiResponse.Code = hogeResp.StatusCode
apiResponse.Description = string(hogeRespBody)
results = append(results, &apiResponse)
}
return results, nil
}
上記処理ではhogeAPIResponse構造体を一度apiResponseという変数に定義して、その空のインスタンスに値をセットしています。このような実装を行なってしまうバグ(値の設定漏れ)が発生しやすくなったり、可読性が下がるため良くないコードです。
値の揃ったインスタンスを一気に作成していきましょう。
results = append(results, &hogeAPIResults{
ID: message["request_id"].(string),
Code: hogeResp.StatusCode,
Description: string(hogeRespBody),
})
}
return results, nil
}
エラー文は使いまわせるように(共通している処理を関数に切り出す)
//修正前
if len(input.LogEvents) == 0 {
return nil, fmt.Errorf("logEvents array is empty")
}
サンプルコードでは上記のようにエラーだった時、nilとfmt.Errorfで返すようにしています。
ただこの実装だと、nilを返さなくてはいけなくなります,,,加えて修正したいときにそれぞれのエラー文で修正しなければならないという手間が発生します。
//修正後
func errorResult(message string, a ...any) (apiResult, error) {
return apiResult{
Code: http.StatusInternalServerError,
Description: fmt.Sprintf("ERROR: "+message+"%v", a...),
}, nil
}
//省略
if len(input.AlarmData.Configuration.Metrics) == 0 {
return errorResult("no metrics found in the input")
}
このように関数に切り出して使いまわせるようにすることで
エラー文を書くだけでよくなりかなり楽になります。
こういった関数に切り出して楽できないかと実装中、意識していきたいところです。
ポイント
毎回書いていて、似ている処理があれば「あれ？似ている箇所あるな,,,共通化できるのでは？」と考えることです。その処理が果たしてたまたまなのか色んなところで書いているのか,,,
色んなところで似たような処理書いてたら共通化するために関数化していきたいところです。
イテレーションに無駄はないか
package main
import "fmt"
type PersonalInformation struct {
Name string
Address *Address
}
type Address struct {
State string
Street string
Country string
PostalCode string
}
func addfamilyMemberInfoBefore() {
familyMemberNames := []string{"Alice", "Bob", "Charlie"}
for _, name := range familyMemberNames {
info := PersonalInformation{
Name: name,
Address: &Address{
State: "hoge",
Street: "fuga",
Country: "jpn",
PostalCode: "piyo",
},
}
fmt.Printf("修正前:%+v\n", info)
}
}
func main() {
addfamilyMemberInfoBefore()
}
修正前：{Name:Alice Address:0xc000024100}
修正前：{Name:Bob Address:0xc000024140}
修正前：{Name:Charlie Address:0xc000024180}
サンプルコードだとわかりづらいのでもっとシンプルでわかりやすい例を作りました。
↑こちらのコードだとforが回るたびに新しいinfoが生成されてしまいます
もっと具体的に
package main
import "fmt"
type PersonalInformation struct {
Name string
Address *Address
}
type Address struct {
State string
Street string
Country string
PostalCode string
}
func addfamilyMemberInfoBefore() {
familyMemberNames := []string{"Alice", "Bob", "Charlie"}
for _, name := range familyMemberNames {
info := PersonalInformation{
Name: name,
Address: &Address{
State: "hoge",
Street: "fuga",
Country: "jpn",
PostalCode: "piyo",
},
}
fmt.Printf("修正前:%+v\n", info)
}
}
func addfamilyMemberInfoAfter() {
familyMemberNames := []string{"Alice", "Bob", "Charlie"}
address := &Address{
State: "hoge",
Street: "fuga",
Country: "jpn",
PostalCode: "piyo",
}
for _, name := range familyMemberNames {
info := PersonalInformation{
Name: name,
Address: address,
}
fmt.Printf("修正後%+v\n", info)
}
}
func main() {
addfamilyMemberInfoBefore()
addfamilyMemberInfoAfter()
}
↑このようにforの外でaddressを定義することでinfoの生成は一回で済みます。
出力するともっと具体的になりわかりやすくなると思います。
修正前:{Name:Alice Address:0xc0001160c0}
修正前:{Name:Bob Address:0xc000116100}
修正前:{Name:Charlie Address:0xc000116140}
修正後{Name:Alice Address:0xc000116180}
修正後{Name:Bob Address:0xc000116180}
修正後{Name:Charlie Address:0xc000116180}
修正前(addfamilyMemberInfoBefore)は全てのアドレスが違っています。全てに新しくメモリが振られています。でも修正後は(addfamilyMemberInfoAfter)全て新しいメモリになってます。つまり修正前の方がそれぞれに新しくメモリが切り出されており、メモリ効率がよろしくない状態になっています。
このようにfor内で無駄な処理がないかセルフチェックの時、レビューの時に意識したいです。
なるべく公式推奨どおりに
main関数にlambda.Start()を実行している箇所があります。
func main() {
lambda.Start(func(ctx context.Context, input *CloudWatchInput) //ここ([]*hogeAPIResponse, error) {
hogeApiResponses, err := hogeApiRequests(ctx, input)
if err != nil {
return []*hogeAPIResponse{{Code: 500, Description: fmt.Sprintf("ERROR: %v", err)}}, nil
}
allSuccess := true
var errorResponses []*hogeAPIResponse
for _, response := range hogeApiResponses {
if response.Code != 200 {
allSuccess = false
errorResponses = append(errorResponses, response)
}
}
if allSuccess {
return []*hogeAPIResponse{{Code: 200, Description: "CREATED: 送信が成功しました"}}, nil
} else {
return errorResponses, nil
}
})
}
Lambdaの公式にはこう書いてあります
参照：https://docs.aws.amazon.com/ja_jp/lambda/latest/dg/go-image.html#go-image-provided
公式と全然違う書き方しちゃってます。
何が言いたいかというと 公式の推奨してある書き方があるのなら特別な理由がない限り、公式通りに書くべきということです。 裏を返せば「公式がこう推奨してあってため」や、命名で迷った時に公式を参考にできるのなら公式通りに命名するっていうのは自分が実装した時の理由としてレビュー時に相手を納得させれる理由になります。
最後に
結局のところpjの方針や場合によりけりで通用したりしなかったりすることがあると思うので、臨機応変な対応が必要だとは思います。ただ最初に書いたように自分が2週間後に読んでもわかるコードを意識して書いていきたいなと思いました笑。