はじめに
どうも、こんにちは。もきおです。
未経験から自社開発企業に転職し半年が経ちました。あっという間の半年間でしたがその間色々ありがたいことにレビューをいただきました。なので今回は半年前の入社した自分に送る言葉的な感じでレビューで指摘されたことを書き起こしておきたいと思います。
最後に対策的なとこも記載しているので最後までご覧いただけますと幸いです。ちなみに弊社はRuby、Railsメインで使用しているのでちょくちょくRubyのメソッドとか出てきます。他の言語使用されている方は適当にあしらってもらえればと笑
指摘1: コードちゃんと読んだ？
これは言われることが最も多かった指摘でしたね。何回かこの指摘を受けた時、コードをちゃんと読むってなん何だろう？と疑問に思ったので具体的なポイントを聞いてみました。
- 分からない箇所を分からないままで読み進めない
- 想像で決めつけて読み進めない
- 一つ一つデバッグして確認する
分からない箇所を分からないまま読み進めない
企業のコードはポートフォリオと比にならない程のコード量ですよねー。そのためついつい各コードの理解が薄いまま読み進めてしまいがちです。急がば回れ？(いや違うか笑)でもないですが丁寧に1行1行実装箇所のコードを読み進めていくことが大切だと思いました。
具体的にいうとdistinct、pluckメソッド等ポートフォリオ作成レベルではあんまり使わないものがバンバン実際の企業のコードでは出てきます。分からない箇所が出てくる毎に「Ruby pluck」とかでググり、どんなメソッドかを確認。
調べても出てこない場合は自社で作成したメソッドだったりするかもしれないのでGitHub上で検索したり、ターミナルにてgrep -rn "検索したい文字列" appとかで実行しひたすらにコードを読み解いていく必要があるのかなと思います。
想像で決めつけて読み進めない
分からない箇所を分からないままにしないと似たような感じではあるのですが、最後までコードを読み進めず、想像でこうじゃないかと勝手に思い込んでしまうこともよくありました。なので
先輩「ここの実装どうなってた？」
もきお「こうなってると思うのですが」
先輩「それってちゃんと確認したの？」
もきお「いや、想像で考えてしまってました」
先輩「想像で読み進めるのやめようね、ここ辿っていくとここに辿り着きそうだね。ここは読んだ？」
もきお「読めてないです」
先輩「想像ではなく地道に読み解いていこう、答えは大体コードの中にあるから」
こんな感じで指摘されたことがあります。
一つ一つデバッグして確認する
コードを読むということは、そこにどのようなデータが渡ってきてるか、どのようなデータを渡しているか、そこら辺も理解して読み進めていく必要があるときが多いです。なので一つ一つデータの受け渡しがコードを見ただけだと曖昧な箇所は、pryで止めて確認する必要があります。
int型(整数型)で渡ってきていると思ったらString型(文字列型) で渡ってきていて上手く実装ができないとかはよくある例かなと思います。
指摘2: インデント or 改行ズレてるね
これは言わずもがなだし最も言われたら恥ずかしい指摘ですよね。弊社はhtmlをslim(htmlを簡潔に書けるテンプレートエンジン)で書いているので意外とインデントどうしたらいいか分からなくなってしまったり最初の頃はしてました。何度もインデントがズレてるというような初歩的なミスを繰り返してしまうとそれだけで先輩にイラッとされるので注意しましょう笑
もちろんRailsだとRuboCop等のコード解析ツールを使用するのも有効です。
指摘3: 他のメソッドは検討した？
言語にもよると思いますが、組み込みメソッドは同じような処理でも沢山あり、それぞれ用途にあう最善のメソッドを選択して使用する必要があります。Rubyだとループ処理一つ取ってもfor,each,mapなど色々なループ処理があります。その時々で最善の選択ができるよう、他にも使えそうなメソッドがないか調べた上で実装するのが良さそうです。
指摘4: これリファクタリングできるよね？
リファクタリングって難しいですよねー。リファクタリング方法は色々とあって
- 処理のまとまりをメソッド化する
- 同じ記述のコードを共通化する
- 条件分岐の中に条件分岐からの条件分岐みたいにネストしまっくってるのを浅くする
まぁ他にも方法は色々とあり、最初はなかなかリファクタリングできそうだけど自分の力じゃ出来無さそうみたいなこともよくあるわけです。ここで先輩に言われてこれなら誰でもできるなと思ったのがリファクタリングできそうな箇所をプルリク時にコメントを残しておくこと。
これは先輩曰く意外と大事らしく。そもそもリファクタリング意思があるかないかでレビュー内容も変わってくるとのこと。どちらにしろなるべくリファクタリングはしたい。よってコメントを残していない場合、「リファクタリングできそうだけど検討した？」というまず確認をしなければいけないためワンクッション挟まないといけない。リファクタリング意思を最初からコメントに残しておけば、具体的にどうリファクタリングしたらいいかレビューすることができ無駄な一ラリーを避けることができるとのことでした。
指摘5: 仕様ちゃんと確認した？
実装前に仕様をちゃんと確認することは実装を効率的に行う上でとても重要だなと思いました。実際にあった事例を元に説明したいと思います。
その時の実装はボタンクリックでのソート機能(検索を絞る)を単数選択→and条件で複数選択できるようにするというものでした。実装前に、これ複数選択できるようにするならレイアウトをボタンじゃなくてチェックボックスにした方がユーザーインターフェース的に良くないか？と思ったのですが、レイアウト変更するくらいだし確認せずまずはボタンで複数選択できるようにしておいて、あとからチェックボックスにするか確認しようと思い実装しました。
無事ボタンによる複数選択が終わり、CTOに「これ複数選択するならチェックボックスにした方がいいですよね？」と聞いたら「その方がいいね、そうしておいて」と言われました。
そしてチェックボックスに変更しようとしたのですがフロント部分がJavaScriptも使用して実装されているためレイアウト変更だけで収まらずJSも大幅に変更する必要が出てきてしまいました。
なので仕様を確認した際、少しでも疑問に思ったことは必ず質問した方が良かったなと思いました。
セルフレビューの大切さ
さて、ここまで色々と指摘されたことを記述してきましたが最後にこれらの指摘をされないために行っている対策を記載して終わりたいと思います。
冒頭でも出ていますが、レビューで指摘を減らすための対策としてプルリクエストを出して先輩にレビューを依頼する前にセルフレビュー(自分自身でもう一度見直す事)を必ず行っています。セルフレビューでは下記のことを意識しています。
- 間違って変なファイルを作成したりしていないか
- メソッド名、変数名等は正しいか
- インデントは問題ないか
- コードが冗長になっていてリファクタリングできそうかどうか
ここら辺は意識して見直しています。また自作メソッド等理解するのに少し時間がかかりそうな箇所に関してはどういった意図、処理なのかを事前にコメントに残すことによって先輩のレビューコストをなるべく下げるようにしています。結構納期で焦ったり、めんどくさかったりしますがセルフレビューは本当に大切だなと感じているので引き続き続けていきたいと思います。
あとがき
いかがだったでしょうか？今回はレビューで指摘されたことを記載してみました。半年以上立ちましたがまだまだレビューで指摘を受けることは沢山あります笑
でも細かくレビューをいただけるのって本当にありがたいですよね。二度あることは三度あることがないように、失敗と改善を繰り返してコードの品質を高めていきたいと思います。少しでも良いと思っていただけたら引き続き記事を書くモチベーションになるのでLGTMポチッと押していただけますと幸いです。