SakuraWi - BLog

みんなのウェディングのエンジニア。聴いたお話をまとめておく倉庫的な。スタックストックスタック!

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


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

f:id:Saku-Saku:20171208161237j:plain

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

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

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

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

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

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

f:id:Saku-Saku:20171207212633p:plain

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

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

やってきました空行問題

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

f:id:Saku-Saku:20171207213011p:plain

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

f:id:Saku-Saku:20171207213208p:plain

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

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

きたるインデント

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

f:id:Saku-Saku:20171207213354p:plain

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

[余談]

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

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

コメントアウトの必勝法

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

ダメダメ変数名

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

f:id:Saku-Saku:20171207214102p:plain

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

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

愛あるパンチ

f:id:Saku-Saku:20171207214427p:plain

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

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

controllerの責務

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

Rails触りたてだと、どこに何を書くべきかあまりわかりませんよね。 f:id:Saku-Saku:20171207214725p:plain

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

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

f:id:Saku-Saku:20171207215100p:plain そしてさらに責務を分けた方がいいのではないか、というレビューが。

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


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

routesで重複問題

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

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

同期からの初レビュー

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

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

f:id:Saku-Saku:20171207221154p:plain

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

伝え方の問題

f:id:Saku-Saku:20171207222245p:plain

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

wipコミット散見

f:id:Saku-Saku:20171208133050p:plain

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

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

不要なテストデータ生成

f:id:Saku-Saku:20171208135152p:plain

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

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

f:id:Saku-Saku:20171208142051p:plain

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

cssの名前

f:id:Saku-Saku:20171208143014p:plain

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

よくあるその他レビュー

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

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

まとめ

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

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