現在絶賛開発中のkirimoriですが、なんとGolang界隈で有名なmattnさんにリファクタリングをして頂くという、とても嬉しい事態がありました✨
kirimoriについてはこちら↓
syossan.hateblo.jp
リファクタリング前提でかなり雑に書いていたのですが、めちゃくちゃ良い感じにコードを直して頂けたので自分の勉強のために読み解いてみます👏
kirimoriは以下の機能を有しています。
で、構成的には 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) {
// 省略
}
ここら辺の話は、この記事の説明が物凄く分かりやすかったです🙇
skatsuta.github.io
追記:mattnさんから訂正が!🙇
コマンド毎の処理をパッケージに分割する
リファクタリング前にはコマンド毎の処理は以下のように書いていました。
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は標準出力なのですね・・・😫
参考記事🙇
qiita.com
path/filepathの使用
設定ファイルのパスをこんな風に処理してました。
setting_file_path string = home_path + "/.kirimori.toml"
こうするよりも path/filepath
のJoinを使った方がスマートでした😥
settingFilePath string = filepath.Join(homePath, ".kirimori.toml")
完全にWindowsをガン無視で作っていましたが、リファクタリングでWindows対応まで付けて頂きました・・・!😂
runtimeパッケージを使って処理を分ければ良いという知見を学びました!
f runtime.GOOS == "windows" {
vimrcName = "_vimrc"
} else {
vimrcName = ".vimrc"
}
golintを使う
これまたlint系も後で良いやーって感じでやってたので、golintでの指摘点を大量に修正して頂いてました😫
また、twitterのフォロワーさんにgometalinterというgoでのlintの集合体を扱うツールを教えてもらいました!
便利なので今後使っていきたいと思います!✨
github.com
プラグインマネージャー毎にパッケージに分割する
プラグインマネージャー毎の処理を scanAddLineForVundle
みたいに For[プラグイン名]
って感じでプラグイン毎に名称を分けてたんですが、こちらもパッケージにそれぞれ分けて頂けました👏
これでまたkirimori.go本体がスッキリ!
引数をos.Fileからio.Readerに修正
NewScannerに引き渡すために各関数の引数をos.Fileにしていたのですが、io.Readerに修正されておりました。
これはテストの際にわざわざファイルを用意してOpenして、ってやるよりかio.Readerを生成して喰わせる方が楽だからなのかな?と思っております。(この辺自信が無いのでどなたかツッコミがあればツッコミ入れて下さい🙇)
変数名を短く
全体的に変数名を短く修正されており、以下の記事にあるようにGolangの文化なのかなと。
blog.sigbus.info
癖で長く変数名書いてるので、短くても伝わるような設計心掛けたいです✨
まとめ
というわけで、初心者Go使いには物凄く学びが多いPRでした!
わざわざ時間を割いてリファクタリングしていただいたmattnさんに多謝🙇