コードレビューが無い世界から、コードレビューがある世界に移った僕が気をつけたい10項目
Tweet
入社1年半にして4部署めに突入しました技術推進室の木村です。 遷移としては
サービス開発チーム(3ヶ月) → スタートアップ室(3ヶ月) → ガールズメディア事業部(1年) → 技術推進室(10月から~)
という感じになってます。
新部署に移りましたが、今後共ご愛顧のほどよろしくお願いします。
ガールズメディア事業部内ではDiaryという日記サービスのAndroid版を一人で作成していましたが、技術推進室ではIRORIというサービスのAPIを、先輩達に囲まれて開発を行ってます。
ガールズメディア時代も2ヶ月ほど別の先輩と一緒に作業していた時期がありましたが、プルリクエストベースでのコードレビューとかはあまり積極的に行ってませんでした。
IRORIでは技術推進室の先輩方に囲まれて新機能をリリースする度にコードレビューをして頂いてます。凄いのとかだと1件のプルリクエストに対して、20件近くコメントをいただきます。言われてその場だけで直しておしまいにすると繰り返してしまうので、先輩方に愛想を尽かされる前に一度今まで頂いたコメントをまとめて注意することを10項目にまとめてみました。
1. とりあえず落ち着け
何はともあれ一呼吸置いてから自分のコードを見なおすことがとても大切だと思います。特に自分は出来たことに嬉しくなって早く誰かに見せたい！という気分になりますが、やっぱり落ち着くことは大切です。なので最近は書き上げた直後とかテンションが上がってるので一旦トイレ行くとか飲み物飲むとかワンテンポ置くようにしようとしています。勢いで出すと大抵トンデモないコードを出してしまうことが多いです。
2. CIはちゃんと通りましたか？
まさかspec、コード規約のテストが通ってないのにコードレビューの依頼を出したりしてないですよね？ええ、そのまさかです。入った当時やらかしました…今考えるとありえないですね。当然のことなので落ち着いてちゃんと処理することが重要です。
3. 使ってない変数とかネストがおかしいものはありませんか？
使ってない変数はツールで発見されますが、プリントデバッグを行った時に残したコードなど、ゴミコードを残してないでしょうか？また、テストなどであるのですが、おかしなネストになってしまっているコードはありませんか？この当たりは落ち着いて見なおせばすぐに見つかりますのでコードレビューで指摘されると恥ずかしいです…というか恥ずかしかったので注意します。
4. それなんで使ったか説明できますか？
なんとなくやってしまうことが多いのですが、なんとなく便利そうだからとか、なんとなくこうしたら動く気がしたので…とかなんとなくで書いたり追加してしまったりすることが多いのですが、なんとなくで書いてしまうと後々で障害になってしまうことが結構あります。エンジニアなのですから、なんとなくでは無くてどうして動くのかといった動作原理や必要性を説明出来るようになる必要があります。
5. そのテスト大丈夫ですか？
コード書いてテスト書いてCIもグリーンになったよし！プルリクエストだ！・・・とその前にその書いたテストは大丈夫でしょうか？テストがグリーンなら大丈夫だろ！とか思ってしまうのですが、そもそも何を渡してもグリーンを返してしまうようなガバガバなテストでは無いでしょうか？特にコードを先に書く習慣がある人ははまりやすいかもしれません。軽く値を変えて確かめてみるだけでもいいのでちゃんと正しい値が返っているかチェックをしたほうがいいと思います。
6. そのテストはひょっとして出力に合わせて記述していませんか？
指摘されるとぐうの音も出ないほど悔しいことですが、時々やってしまうことがあります。テストの目的はグリーンにすることではなくて、自分が行った作業が仕様通りに実装できているかの確認作業だと思ってます。特に怪しいことを書いてしまうのがデータベース項目の昇順、降順を並べ替えた時などです。これらも注意されないよう。常に考えていきたいところです。
7. それは冗長な処理 or 過度な最適化ではありませんか？
良くやりがちなのは、もっと簡単に便利に書ける処理があるのに冗長すぎるコードを書きすぎることです。あまりに処理が長くなりすぎると、レビュワーが本質的な問題を見逃してしまう可能性が高いです。また、過度な最適化を進めすぎて一行になんでも詰め込みすぎてしまうと、それはそれで読みにくくなります。一週間後の自分が読んだとして読みやすいものを作っていきたいものです。
8. その特殊な処理はメソッド化しなくて大丈夫ですか？
例えばですが、データベースはNULL制約がかけられてないけど、コード内ではNULLを認めないようなコードを書いしまった時に生成等に使うメソッドを用意する必要があります。妙に具体的な話ですが、これで先輩の足をおもいっきり引っ張りました。その時はどちらでもいいかな？と思ったのですが、やっぱりちゃんとしておけばよかったと後悔した思いがあります。どうしても変なコードを書く場合にはそれが出来ないやりにくいような仕組みを作ることが大切なんだなと思いました。
9. そのコードは読みやすいですか？説明は足りてますか？
複雑なメソッドや冗長な処理は出来る限り書かないようにしたいものですが、やはりどうしても難しいSQLを書かなくてはいけなかったり、やりたいことに合わせて書いていくとコードがどんどん複雑になり、ひと目で理解できないものが生まれてしまうことがあります。そういう時は最低限わかりやすくするため簡単にコメントを残すようにしましょう。
また、処理の区切れ目に適切な空白を入れておきましょう。文章が長くなると読むのが辛くなるようにコードも長くなると読むのが辛くなります。空白などで適切にブロック化してあげるだけで読みやすさが段違いになります。
10. やりたかったことはそれで出来てますか？
最後に一番大切なことですが、その書いたコードでやりたかったことは出来てますか？求めていた処理はちゃんと書けてますか？そのコードは動くけど欲しかったものとずれてないでしょうか？事前に確認しておいてもずれるときもあります。常に注意したいものです。
と、いうわけでこの3ヶ月間先輩に注意を受けてきたことを10個にまとめてみました。
ちゃんとしたエンジニアはこういったことがちゃんと出来る人だということを理解しました。と言うよりは今まで酷いコード書き散らかしてきちゃったなー…という残念な感じが拭えないです…
どれも一つ一つは難しいことではないので、ちゃんときっちりとこなせるようになりたいと思ってます！
今後はこれを印刷してコードレビュー前に見て自分で問題点を抜き出せるようにしたいと思います。
こんな当たり前のことはすぐに出来るようになって、新しいことをどんどん吸収して行きたいと思ってます！
