ここ半年の間にコードレビューで受けたコメントなどをまとめた
レビューで指摘されたり教えてもらった内容をまとめた。
命名
- プロダクト全体の命名規則にあっていない。
create_xxxというメソッドが DB にレコードを作るのであれば、createはそのような関数の命名のみに使うべき。 get_xxxという命名が多い。
get以外にもfindやfetchなどが使える。- 名前が抽象的。
- パッケージ名が大きすぎる。
名前が適切でないというコメントを何度か受けている。機能を十分に表す名前が付けられていないという指摘である。get だらけになってしまうのも抽象的な命名の現れだと思う。
抽象的な命名をしてしまうのは視野の狭さが原因だと思う。実装している部分だけで通る名前をつけてしまっている気がする。限られた範囲だけなら抽象的な名前でも表すものは一意に決まるので、この問題に気が付けないのかも。
そう考えると、「プロダクト全体の命名規則にあっていない」も納得がいく気がする。狭い範囲だけで考えてしまうから、プロダクト全体の規則から逸れた命名をしてしまうのではないか。
どうする
「ディレクトリ構成を把握する」とか「クラスや関数を眺めてみる」とかしか思いつかない。何か仕組みを作って解決するとかじゃなくて、自分が気をつけるしかない気がする。
Self code review みたいな概念がありそう。考慮し忘れるという問題に対しては、あらかじめ用意したチェックリストを見ながら自分でコードレビューすれば良さそう。
設計・実装
- インタフェースから使い方がわからない。
- 前提をコメントに書いておくと安心。
- 変数や関数に切り出すと、切り出した意味があるんだろうと考えられる。
名前つけて置いてるということはなにか特別な用途があるのかな？ と思ってしまう。
1. インタフェースから使い方がわからない
これは命名にも関係していそう。ここで指摘された関数は、呼び出す側の都合があって戻り値がやや特殊な形になっていた。この関数単体で見ると何を返しているのかが分かりにくいという問題があった。
結局は HashRef （とかオブジェクトとか辞書とか Map とか呼ばれたりするもの）にして、Key で値がなんなのかわかるようにした。情報量を増やしてより説明的にすることで、この問題に対処できる気がする。
2. 前提をコメントに書いておくと安心
コードからは読み取りにくい前提条件があったので、そういうのはコメントしておくのが良いという指摘。自分だけが知っていそうだなと思った部分には細く説明をしておく。もちろん、コードからすべてを読み取れるなら、それに越したことはない。
3. 切り出す意味は何か
関数や変数を切り出すのにはいくつか理由がある。共通部分を切り出して別のところでも使うとか、説明的な名前をつけてコードをわかりやすくするとか。逆にいうと、別のところで使わないし、説明的な名前でもないなら切り出す必要はない。
意図的にインラインのままにしておくのはあり。
Perl
要素数 - 1 を得る
scalar @$contents - 1 は $#$contents と書ける。
戻り値を捨てる
my ($hoge, $fuga) = return_hoge_and_fuga();
my (undef, $fuga) = return_hoge_and_fuga();いらない戻り値は undef で受け取れば良い。
JSON
JSON::Types::bool() などを使う。
MySQL
`created_at` TIMESTAMP NOT NULL DEFAULT 0はモードによっては許可されない。DEFAULT CURRENT_TIMESTAMP などにするとよい？
例えば、sql_mode の TRADITIONAL に STRICT_TRANS_TABLES と NO_ZERO_DATE が含まれている。
このモード(NO_ZERO_DATE)および厳密モードが有効な場合、IGNORE も指定されている場合を除き、'0000-00-00' は許可されず、挿入によってエラーが生成されます。