ダメダメだったコードを修正してみた【考え方】

# この記事を書いた目的
以前に社内で先輩に自分が書いたコードを丁寧にレビューしてもらったことがあります。

この時に学んだ教訓を忘れないようにしっかり記事にアウトプットしてまとめようと思ったのが目的になります。

基礎的な内容だったり、当たり前と思ってる人からするとなんてことはない内容かもしれませんが、意外とこういったコードをより良くするための記事や本って少ない気がします,,,
(実務で得たリファクタの知見)

私自身も「強い人ってこういった観点でコードをレビューしているんだ」って
とても学びになりました。

ぜひ誰かの参考になれば幸いです。

書いてたら長くなったのでコードを書くときにより読みやすいコードを書くための「考え方編」とGolangで書くときにより読みやすく書くための「golang編」に分けました。

この記事では「考え方編」を書いてます。

# 今回取り扱うサンプルコードについて
今回以下のコードを改善していきます。
取り扱う内容として、レビューで指摘もらった箇所を解説していきます。

ちなみにこのソースコードが何をしているかというと
hogeAPIから受け取ったJSON構造体によってリクエストが成功したのか失敗なのかをLambdaを用いて通知するシステムになります。

```golang:main.go //修正前
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
		}
	})
}
```

https://qiita.com/come3300/items/d5c147aa92cfeb36b8ee

似たようなシステムで上記記事も書いてます

# リファクタした点
コードを書くときの基本的な考え方として
「他の人が読んでも違和感なく、読みやすいコードを書く」
自分が実装したコードがなぜそう実装したのか説明できるように意識すると良いと教わりました。

他にも以下を意識すると良いと。
- 可読性を求めてコードを書く→正解を探して書くのではない
- 他の人が読んでも違和感、混乱させないコード
- 何かルール作って愚直に書くと読みづらくなることもある
- 2週間後の自分が理解できるか
    - そこに懸念があると改善の余地あり

具体的にどういったことになるのかというのを説明していきます。
※我もの顔で解説しますが、先輩エンジニアに指摘された内容(受け売り)であることをご了承ください笑

# 変数名は名詞、関数名は動詞
```golang
func hogeApiRequests(ctx context.Context, input *CloudWatchInput) ([]*hogeAPIResponse, error) {
```
この関数名は「`[]*hogeApiRequest`のようなインスタンスに思えて」分かりづらいと指摘をもらいました。

「`変数(構造体)名`を定義するのに`関数like`な命名は読みづらい,,,,操作(関数)的な印象を受けますよ」と。
逆も然りです。(`関数名`で`変数like`な命名だと変数的な印象を受ける)

- `func handleCloudWatchLog`

上記に修正を加えました。
ポイントは(基本的に)
- 変数名は名詞
- 関数名は動詞(操作的な命名)

を意識して定義する

# 可読性の下がる省略をしない
変数名を定義している時に`req`や`i`など変数名を省略して書くことってあると思います。
今回のコードだと以下の部分になります
```main.go
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`なのかとても追いづらいと思いませんか？

省略することでこう言ったリスクが生じます。

結局省略していいのかの判断基準として**実態が掴めるのか,掴めないのか**が重要になってくると教わりました。

**その変数、関数が何を目的としているかの実態が掴めるように命名していくこと**

# わかりづらい変数名(実態が掴みづらい)
変数を定義するときに意識することは他の人が読んだ時に読み手に伝わりやすいコードを書くことです。
そのためにもシンプルで伝わりやすい変数名にすることが大事です。

```golang
type hogeAPIResponse struct {
	ID          string `json:"id"`
	Code        int    `json:"code"`
	Description string `json:"description"`
}
```
今回のコードであればこちらです。(変数ではなく構造体ですが,,,)
この構造体はhogeAPIからのレスポンスのための構造体ではなく、この型が表現したいのは、`外部APIから受け取った値を処理した結果`です。なので本来の意図と書いてる命名に違和感があります。

よって
```golang
type hogeAPIResults struct {
	ID          string `json:"id"`
	Code        int    `json:"code"`
	Description string `json:"description"`
}
```
と修正しました。
意図として「API送信を含めた処理の結果」が伝わるように命名したというのがあります。

ポイントは
**変数名から目的と型が一目で分かること**
が大切になるのかなと思います。

### 関連記事

記事を書きながら調べてくうちに言いたいことが簡潔に書いてある記事を見つけたので紹介しておきます。

https://qiita.com/kimisyo/items/3d56d9dc3a2011c87e3d

# publicとprivateの使い分け

```main.go
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`って定義しているのに使っているのは最後の方です。

```main.go
func hogeApiRequests(ctx context.Context, input *CloudWatchInput) ([]*hogeAPIResponse, error) {
var results []*hogeAPIResponse

,,,省略 //hogeApiRequestsの最後らへんに

results = append(results, &apiResponse)//定義して数十行後に使っている
	}
	return results, nil
}
```
こうしてしまうと読んでいくうちに「あれ、`results`って何だっけ？」と読みづらくなり、可読性が下がるのでやめましょう。

**変数の定義は使用する直前に書く**

# なるべく変数は定義しない方針で

```main.go
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`という変数に定義して、その空のインスタンスに値をセットしています。このような実装を行なってしまうバグ(値の設定漏れ)が発生しやすくなったり、可読性が下がるため良くないコードです。

値の揃ったインスタンスを一気に作成していきましょう。
```main.go
results = append(results, &hogeAPIResults{
			ID:          message["request_id"].(string),
			Code:        hogeResp.StatusCode,
			Description: string(hogeRespBody),
		})
	}
	return results, nil
}
```

# エラー文は使いまわせるように(共通している処理を関数に切り出す)
```diff: main.go
//修正前
	if len(input.LogEvents) == 0 {
		return nil, fmt.Errorf("logEvents array is empty")
	}
```
サンプルコードでは上記のようにエラーだった時、`nil`と`fmt.Errorf`で返すようにしています。
ただこの実装だと、`nil`を返さなくてはいけなくなります,,,加えて修正したいときにそれぞれのエラー文で修正しなければならないという手間が発生します。
```main.go
//修正後
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")
	}
```
このように関数に切り出して使いまわせるようにすることで
エラー文を書くだけでよくなりかなり楽になります。

こういった関数に切り出して楽できないかと実装中、意識していきたいところです。

### ポイント
毎回書いていて、似ている処理があれば「あれ？似ている箇所あるな,,,共通化できるのでは？」と考えることです。その処理が果たしてたまたまなのか色んなところで書いているのか,,,

色んなところで似たような処理書いてたら共通化するために関数化していきたいところです。

# イテレーションに無駄はないか
```golang:サンプルコード
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`が生成されてしまいます

もっと具体的に

```golang
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()`を実行している箇所があります。
```main.go
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の公式にはこう書いてあります
![スクリーンショット 2025-02-07 1.25.37.png](https://qiita-image-store.s3.ap-northeast-1.amazonaws.com/0/705510/9a087d81-bef1-b20e-4935-0a1cc31ed1ec.png)

参照：https://docs.aws.amazon.com/ja_jp/lambda/latest/dg/go-image.html#go-image-provided

公式と全然違う書き方しちゃってます。
何が言いたいかというと **公式の推奨してある書き方があるのなら特別な理由がない限り、公式通りに書くべき**ということです。 裏を返せば「公式がこう推奨してあってため」や、命名で迷った時に公式を参考にできるのなら公式通りに命名するっていうのは自分が実装した時の理由としてレビュー時に相手を納得させれる理由になります。

# 最後に
結局のところpjの方針や場合によりけりで通用したりしなかったりすることがあると思うので、臨機応変な対応が必要だとは思います。ただ最初に書いたように自分が2週間後に読んでもわかるコードを意識して書いていきたいなと思いました笑。