リファクタするのって難しいね

リファクタリングってしていますか？
取り急ぎ動くものを作ったけれど後から見ると色々改善できたりすることあるのではないでしょうか。
簡単に自分のリファクタリング例を載せていきたいと思います。

処理としては、
ユーザーをID検索し、退会済のユーザーであればエラーを出力するようにします。

package main

import (
	"fmt"

	"golang.org/x/xerrors"
)

// user ユーザー定義
type user struct {
	name     string
	id       uint
	userType uint
}

// userList ユーザーリスト
type userList []*user

// filterByIDs ユーザーリストからidで対象のユーザー検索
func (list *userList) filterByIDs(ids []uint) userList {
	out := userList{}
	for _, v := range *list {
		for _, id := range ids {
			if v.id == id {
				out = append(out, v)
			}
		}
	}
	return out
}

// invalidType ユーザータイプの判定
func (user *userList) invalidType() bool {
	for _, v := range *user {
		if v.userType == 2 {
			return false
		}
	}
	return true
}

func main() {
	if out, err := test(); err != nil {
		fmt.Printf("failed to test() : %v", err)
	} else {
		for _, v := range out {
			fmt.Print(v.name)
		}
	}
}

// test テスト用関数
func test() (userList, error) {
	// テスト用データ定義
	users := userList{
		{name: "kinoko", id: 1, userType: 1},
		{name: "takenoko", id: 2, userType: 1},
		{name: "suginoko", id: 3, userType: 2},
	}
	ids := []uint{2}
	// 有効なユーザーか判定する
	if out := users.filterByIDs(ids); !out.invalidType() {
		return nil, xerrors.New("invalid user")
	} else {
		return out, nil
	}
}

上のコードを見て、いくつくらい改善箇所見つかりましたか？
今回は3つ取り上げさせていただきます。

レシーバ名

goにおいてレシーバ名は短く1文字あるいは2文字程度の省略形とすべきです。
また同様のレシーバは一貫性を持った名前にしなくてはならないのでメソッドによってバラバラの命名をしてはいけません。
よってuserList型の構造体レシーバを使って、メソッドを定義しているfilterByIDs(),invalidType()をそれぞれ下記のように修正します。

func (l *userList) filterByIDs(ids []uint) userList {
	out := userList{}
	for _, v := range *l {
		for _, id := range ids {
			if v.id == id {
				out = append(out, v)
			}
		}
	}
	return out
}

func (l *userList) invalidType() bool {
	for _, v := range *l {
		if v.userType == 2 {
			return false
		}
	}
	return true
}

定数

コード内で意味のある数字や文字列を使用する際は必ず定数化してから使用しましょう。

const unSubscribe = 2

func (l *userList) invalidType() bool {
	for _, v := range *l {
		if v.userType == unSubscribe {
			return false
		}
	}
	return true
}

エラーハンドリング

エラーハンドリングの分岐はなるべく最小になるようにします。
そのためmain(),test()はそれぞれ以下のように修正します。

func main() {
	out, err := test()
	if err != nil {
		fmt.Printf("failed to test() : %v", err)
	}
	for _, v := range out {
		fmt.Print(v.name)
	}
}

func test() (userList, error) {
	users := userList{
		{name: "kinoko", id: 1, userType: 1},
		{name: "takenoko", id: 2, userType: 1},
		{name: "suginoko", id: 3, userType: 2},
	}
	ids := []uint{3}

	out := users.filterByIDs(ids)
	if !out.invalidType() {
		return nil, xerrors.New("invalid user")
	}
	return out, nil
}

まとめ

今回は基本的な部分を記載致しましたが、
上記３つの他にも本来であればモデル定義と処理で別パッケージにし、ディレクトリ構造も分けられますし、変数名、メソッド名も人によってはこうした方が良いなどあるかと思います。
リファクタを重ねてより品質の良いコードを作成していきたいですね。

全然関係ない話ですが、明治製菓さんの有名なお菓子「きのこの山」、「たけのこの里」には他に「すぎのこ村」というのが昔あったらしいですよ。
みなさんご存知でしたか？

参考

https://knsh14.github.io/translations/go-codereview-comments/#indent-error-flow
https://github.com/golang/go/wiki/CodeReviewComments#receiver-names