現在絶賛開発中のkirimoriですが、なんとGolang界隈で有名なmattnさんにリファクタリングをして頂くという、とても嬉しい事態がありました✨
kirimoriについてはこちら↓
リファクタリング前提でかなり雑に書いていたのですが、めちゃくちゃ良い感じにコードを直して頂けたので自分の勉強のために読み解いてみます👏
リファクタリング前
kirimoriは以下の機能を有しています。
- initコマンドでkirimoriの設定ファイル（toml形式）を作成します
- addコマンドでコマンドライン引数に指定したプラグインを追加します
- removeコマンドでコマンドライン引数に指定したプラグインを削除します
- listコマンドでプラグインの一覧を表示します
で、構成的には kirimori.go に全てのコマンドの処理をベタ書きにしてある感じになっております。
リファクタリング後
細かく読み解く前に、mattnさんによってリファクタリングされた完成形がこちらになります。
かなり構成が変わってますね・・・如何にリファクタリング前のコードが酷かったのか窺い知れます🤔
では、読み解いていきましょう。
ポインタレシーバを使う
ASTのVisitor構造体に対してVisitメソッドを定義する際に以下のように値レシーバを用いていました。
func (v AddVundleVisitor) Visit(node ast.Node) (w ast.Visitor) {
// 省略
}
ただし、値レシーバを用いた場合にはnilのポインタ変数でメソッドを呼び出すとpanicが発生しますが、ポインタレシーバの場合は発生しません。（構造体のフィールドをメソッド内で参照している場合はpanicが発生します）
なので、以下のようにポインタレシーバを使うようにリファクタリングされておりました。
func (v *AddVundleVisitor) Visit(node ast.Node) (w ast.Visitor) {
// 省略
}
ここら辺の話は、この記事の説明が物凄く分かりやすかったです🙇
追記：mattnさんから訂正が！🙇
ちなみにリファクタリングの記事のポインタレシーバの件、少し意味が違ってて構造体メソッドがフィールドの値を更新する場合はポインタじゃないと出来ないのですよね。 https://t.co/Lf5knCOnhn
— おののいも夫 (@mattn_jp) 2017年1月23日
コマンド毎の処理をパッケージに分割する
リファクタリング前にはコマンド毎の処理は以下のように書いていました。
func makeApp() *cli.App {
app := cli.NewApp()
app.Name = "kirimori"
app.Usage = "Add Vim Plugin Tool"
app.Version = "1.0"
app.Commands = []cli.Command{
{
Name: "init",
Aliases: []string{"i"},
Usage: "create setting file",
Action: func(c *cli.Context) error {
// 40行ほどの処理
},
},
{
Name: "add",
Aliases: []string{"a"},
Usage: "add plugin",
Action: func(c *cli.Context) error {
// 80行ほどの処理
},
},
{
Name: "remove",
Aliases: []string{"r"},
Usage: "remove plugin",
Action: func(c *cli.Context) error {
// 70行ほどの処理
},
},
{
Name: "list",
Aliases: []string{"l"},
Usage: "list plugin",
Action: func(c *cli.Context) error {
// 40行ほどの処理
},
},
}
return app
}
この処理を書きながら「さすがに可読性が悪いよなぁ・・・」と思ってましたが、この一連のコマンド処理をpackageに分割されてました。
func makeApp() *cli.App {
app := cli.NewApp()
app.Name = "kirimori"
app.Usage = "Add Vim Plugin Tool"
app.Version = "1.0"
app.Commands = []cli.Command{
{
Name: "init",
Aliases: []string{"i"},
Usage: "create setting file",
Action: cmdInit,
},
{
Name: "add",
Aliases: []string{"a"},
Usage: "add plugin",
Action: cmdAdd,
},
{
Name: "remove",
Aliases: []string{"r"},
Usage: "remove plugin",
Action: cmdRemove,
},
{
Name: "list",
Aliases: []string{"l"},
Usage: "list plugin",
Action: cmdList,
},
}
return app
}
こっちの方が断然見やすいですね！
printlnからfmt.Printlnへ修正
これ全く知らなかったんですが、ビルトイン関数のprintやprintlnは書き込み先が標準エラー出力でfmtパッケージのPrintやPrintlnは標準出力なのですね・・・😫
参考記事🙇
path/filepathの使用
設定ファイルのパスをこんな風に処理してました。
setting_file_path string = home_path + "/.kirimori.toml"
こうするよりも path/filepath のJoinを使った方がスマートでした😥
settingFilePath string = filepath.Join(homePath, ".kirimori.toml")
Windows対応
完全にWindowsをガン無視で作っていましたが、リファクタリングでWindows対応まで付けて頂きました・・・！😂
runtimeパッケージを使って処理を分ければ良いという知見を学びました！
f runtime.GOOS == "windows" {
vimrcName = "_vimrc"
} else {
vimrcName = ".vimrc"
}
golintを使う
これまたlint系も後で良いやーって感じでやってたので、golintでの指摘点を大量に修正して頂いてました😫
また、twitterのフォロワーさんにgometalinterというgoでのlintの集合体を扱うツールを教えてもらいました！
便利なので今後使っていきたいと思います！✨
プラグインマネージャー毎にパッケージに分割する
プラグインマネージャー毎の処理を scanAddLineForVundle みたいに For[プラグイン名] って感じでプラグイン毎に名称を分けてたんですが、こちらもパッケージにそれぞれ分けて頂けました👏
これでまたkirimori.go本体がスッキリ！
引数をos.Fileからio.Readerに修正
NewScannerに引き渡すために各関数の引数をos.Fileにしていたのですが、io.Readerに修正されておりました。
これはテストの際にわざわざファイルを用意してOpenして、ってやるよりかio.Readerを生成して喰わせる方が楽だからなのかな？と思っております。（この辺自信が無いのでどなたかツッコミがあればツッコミ入れて下さい🙇）
変数名を短く
全体的に変数名を短く修正されており、以下の記事にあるようにGolangの文化なのかなと。
癖で長く変数名書いてるので、短くても伝わるような設計心掛けたいです✨
まとめ
というわけで、初心者Go使いには物凄く学びが多いPRでした！
わざわざ時間を割いてリファクタリングしていただいたmattnさんに多謝🙇
