コードレビューについて考える

いよいよあと二日で検証環境にリリースだぜ!
今月残業100hこえたぜ!死にそうだぜ!
こんばんは、お久しぶりです。カバサワです

コードレビューについて考える

最近、ほぼほぼ新卒3人+αのコードのレビューに1日の1/3くらい使ってます。
来月から3年目になるということもあって、一旦ですね、二年前の自分のような初心者に向けたプルリクを出すときの心構えや、プルリクについて色々振り返ろうと思う


ここから心構え編

1.テストをちゃんと通してあること

そもそもの話として、動かないコードをレビューしても意味がない。
以前あったのが、「一番厄介な処理は書いてないんですけど、プルリクを送りました!」
っていうのがあって、一度レビューを通したものの、後にそこの修正がコード全体に影響を出してほとんど構成が変わったコードで再度プルリクを出されて、めっちゃ時間の無駄を感じた。
最低限、考えられる正常系だけでも通してもらわないと、コスト的にはそぐわないかな、と

2.不要な変更が入っていないこと

リファクタしながらやってる 技術的負債を返済しながらやってると聞くと聞こえはいいけど、個人的にリファクタは優秀な人が本腰入れてやらない限りデグレが最も発生しやすい行為だと思う。
今日あったのが、ここエラーでるから少し処理を変えたっていうので、そこコメントアウトすると他の画面で動かなくなるんだけど。。。っていう奴
新入りの君の知見よりは実績のあるコードの方が信頼できるとは思わんかね

3.レビューもらったところはきちんとなおす

レビューもらったら適切に議論すべきだと思う。レビューしてくれる人が必ず正しいとは限らないので。
ただ、レビュー受けたところを無視して修正コミットしてこないでください。
コメント全部読んで、理解できないところは議論して、納得して修正してもう一度出してきてください。
その際ちゃんと1.2ができているか確認してきてね。



ここからコード編

4.未使用変数や未使用関数がないこと

少なくても小さい修正に対して、使っていないコードが存在するのは悪だと思う。
基幹部分の変更や新規機能開発のためにTODOがのこっている場合は別として、いらないコードはそれだけでレビューの妨げになるからやめてほしい

5.無駄に変数をつかいまくる

配列操作の時などにもよくあるけど、無駄に$tmpを用意してしまう奴
普通にみずらいし、命名がカオスになってくるから不要な変数は用意しないでほしい

6.謎ネーミングセンス

フレームワークやコード規約に沿って適切な名前をつけてください。
空気だけじゃなくて、他のコードも読んでください。
tmp_idArrayとかいう名前見たときは素直に引きました。なんだそれ
あとデータベースのカラム名にlabel1, label2とかいう謎命名するのやめてください、なんだこれ。

7.マジックナンバー祭り

countArray == 20とかやめてくれ
DEFAULT_NUMとか定義してくれ
個人的にはactiveFlg == 0もゆるせない
せめてactiveFlg == IS_ACTIVE とか isActive == trueとかにしてくれ

8.関数長杉内

validationが長くなるなら、別関数用意するなり、フレームワークが用意しているものを使ってくれ
よみずらい、デバッグしずらいと本当にひどいことになるので、とにかく簡潔に書く
なるべく簡単に書こうとすれば、自ずと短い関数で構成された読みやすいコードが欠けると思う

9.スコープ広杉内

とりあえずグローバル変数で持たせればおk的発想はやめましょう
思いもよらぬ箇所で変更が入って、処理を追うのが難しくなります。だいたいJSのバグってこれだと思う

10.Gitを理解してくれ

「プルリク出しました!リリースブランチにプッシュしておきました!」って言われてファッ!?ってなった。gitの基本的なコマンドはもちろん、なんのためにブランチを分けて、何のためにプルリク出しているのかを理解してほしい。「push -fしたら直ります!」じゃねぇよ。荒療治ってレベルじゃねーぞ



他にも細かいところはいっぱいあるんだけど、自分がコードレビューで指摘するのはこの辺が多い印象
こっちの実装の方が早いよ!なんて言える難易度のものじゃなくて、本当に簡単な修正に関してもこんだけのことがボロボロとでてきます。

新卒のときの気持ちを忘れずに引き続き精進を重ねていきます