新卒入社の僕が半年で受けたコードレビューとコメント

みんなのウェディング Advent Calendar 2017の８日目の記事です。

みなさんこんにちは、新卒エンジニアの櫻井です。 2017年4月にみんなのウェディングに入社し、8ヶ月が経過しました。

エンジニアの研修が6月から始まったので、ちょうどエンジニアの業務に携わって半年が経過したことになります。

今日の記事は、そんな新卒の僕が受けたコードレビューとgithub上でのコメントを集めてみました。 ためになるレビューなのか、おもしろレビューなのか、さてはて。

前半は、RailsTutorialを進める上でいただいたレビューになります。(ありがたや)

記念すべき業務初コメント

いただいた初のコメントがこちら。

LGTM。今となってはなじみのある言葉ですが、当時は何のことを言ってるのかさっぱりでした。

PRを出して一つ目のコメントがシュっとLGTMをいただけるようになりたいと思っております。

LGTMとは : Looks good to meの略で、「よさそうです！」「OKです！」「さっさとマージしてください！(違)」の意味です。

やってきました空行問題

さあさあ来ました、ファイル末尾の空行があるかないかという問題です。 エディタの設定でも自動化されておらず、ついつい空行なしでpushしてしまっています。

そんなレビューに対しての私のコメントがこちらです。

おっとPendingだ！ なんと驚きのpending。まだpendingです。 相手に伝えたい内容だけ書いて伝えずに終わっています。せつないです。

githubに慣れていなかった自分は、こんな状態でした。

pendingはまとめてコメントを最後に反映させますという状態で、「コメントする」というものを押さないとみえるようにならないのですね。レビューを受けた、した、というときになりがちなので注意しましょう。

きたるインデント

これも誰もが通るであろう、インデントの問題です。 各言語によってある程度インデント(スペース)の数は決められてることが多いです。中にはタブで指定されている環境もあるかもしれません。

ばっちりとレビューを受けています。 おかしいです、という指摘だけではなく、正しくはどうするべきなのか、またどうするとこのような問題が解消するのかについてもコメントしてもらっています。

[余談]

今年の新卒エンジニアは5人入社しており、このインデント問題はのちに複数人引き起こすことになります。

みかねた上司は、rubocopを導入せざるを得ない状況となり、rubocop導入講座が開かれるのでありました。

コメントアウトの必勝法

コメントアウトにおいてもこうした方が良い、という指摘もいただいております。 コメントアウトの方法によってコードがどう変化するのかについても教えていただいています。

ダメダメ変数名

個人開発のなごりのようなものだと思います、変数名よくわからない問題です。

リーダブルコードにも同様の内容が記載されていたと思います。 第三者の可読性を上げるためにも、変数名には何のために使用している変数なのかがみただけでわかるようにしよう、と心に決めました。

12月の現在でも変数名については迷う時が多々あります。 英語の言葉選定の問題であったりした場合には、複数人の英語が強い人に聞いてみてから決めたりしています。

愛あるパンチ

愛のあるパンチです。 テスト通すのは大事です。赤い状態で放置しないようにしましょう。

また、レビューをお願いする時はきちんとテストが通った状態でお願いするのが一般的だと思います。(反省)

controllerの責務

とにかくなんでも同じコントローラーに入れてしまう問題ですね。

Rails触りたてだと、どこに何を書くべきかあまりわかりませんよね。

この場合でいうと、static_pages用のコントローラーにsearchメソッドを生やしてしまっていますね。 それに対して、疑問を投げかけてもらっています。

このあとのやりとりをもって、 search_controllerを作ることになります。

そしてさらに責務を分けた方がいいのではないか、というレビューが。

当時正直あまり理解していませんでしたが、今なら分けた方が良いというレビューに対して納得です。 それぞれの何のためのコントローラであるかが明確になりますね。

この辺の話はオブジェクト指向の考え方や、そもそもRailsならこうすることが多いという経験を積むことで理解が進んでいくと思っています。

---

ここまでがRails Tutorialでの受けたコードレビューになります。 さて、ここからは個人作成アプリのレビューコメントになります。

routesで重複問題

Railsを触りたてだと正直言って、routesの書き方がいまいちわかっていませんでした。 resourcesによってどのroutingが生成されていることを理解していないがために受けたレビューですね。

rails routesコマンドを叩いて確かめましょう！

同期からの初レビュー

findについて見つからない場合.destroyができないので、場合分けをして対応するべきなのでは、というレビューですね。 同期同士でもビビらずにレビューしていこうよ！と、自分からどんどんレビューしてきてくれたのが、彼、武田くんです。

そして自分のコメント返しがなかなかにひどいですが、そこは目を瞑りましょう。

そして来たるrubyっぽい書き方。

ちなみに、.findだと見つからない場合に例外は発生してエラーページが表示されます。.find_byだと見つからない場合にnilが返り値として返ってくるため、ifによる処理で場合わけができますね。

伝え方の問題

ちゃんと調べて、理解して相手に伝えないとだめだよ、という指摘ですね。 このようなコメントで指摘してもらえる環境自体がありがたいことだなぁと思います。

wipコミット散見

commitを積む際にwipしておいたものを忘れてpushしてしまうことがありました。

git rebase -iで一網打尽にしましょう。そして綺麗にコミットを積み上げましょう。

不要なテストデータ生成

これはfactorygirlを使用してテストを書いていた時のレビューで、必要のないものは省こうというレビューですね。 page nationさせるならともかく、生成が2件ほどで済むものは極力少なくすることで少しでもテストのスピードをあげるようにするべきだと学びました。

一次変数をうまく取り扱う

ループの中で何度もメソッドを使って呼び出したりしてしまうケースがあれば、一次変数に格納して処理を軽くしましょう。

cssの名前

これ結構あるあるかと思うのですが、使っちゃいけない名前ってありますよね。 不具合を防ぐためにも、それぞれclass名はしっかりとつけてあげてスタイルを反映させましょう！

よくあるその他レビュー

業務的な内容が出てくるレビューについてはスクリーンショットがとれそうにないので、箇条書きにしてまとめてしまいます。

• テストないよ
• テスト通してね
• Decoratorにしよう
• 適切な命名になっているか？

まとめ

githubを通していただいたコードレビューは見返すことができます。 マージされたり、クローズされるともう見返す機会はほとんどないと思いますが、大事な気づきであったりおろそかにしている部分がレビューとして残っているはずです。

みなさんもぜひ、今年中に一度受けたレビューを見返して「あの時こんなレビューもらったな」と思い返してみてはいかがでしょうか。