新卒１年目でもらったコードレビューを振り返って血肉とする

Transcript

1. 新卒１年目でもらったコードレ ビューを振り返って血肉とする Kairi Yasumatsu Press Space for next page ナイル株式会社 メディアテクノロジー事業部　ピタッとROAS 　エンジニア
2. Who I am ？ 安松　海里 ナイル株式会社：　メディアテクノロジー事業部　(20 卒入社) （インターン：2019 夏くらい〜） 💻 職種： エンジニア　 Appliv ( インターン) ピタッとROAS 　（入社〜） 🧑‍💻　趣味： 個人開発・プロ野球・NBA ・海外サッカーとか　 ※ 今回のLT では、マークダウンに慣れている訳でもないのに、slidev を使用しております Press Space for next page
3. 目次 1. コードレビューについて 2. もらったコードレビューを分類した 3. レビューを振り返る 3.1: コードの再利用性について 3.2: コードの可読性について 3.3: パフォーマンスへの意識 Press Space for next page 新卒１年目でもらったコードレビューを振り返って血肉とする
4. 1. コードレビューについて チーム内でタスクの洗い出し 要件定義 実装 GitHub でPR （プルリクエスト）を投げる レビューを受ける OK が出るまで修正する LGTM! 今回はレビューを分類し、その中で印象に残っていたり、重要だなと思ったレビューをピックアップしました Press Space for next page 今の業務ではGitHub を用いたコードレビューをしています。おおまかな開発フローは以下のような感じです。
5. 2. もらったコードレビューを分類した 今までに私がGitHub 上で出したPR の数はインターン期間を含めた約２年間で、合計292 個でした。その中か ら、直近のピタッとROAS での実装で、特に多かったレビューを大まかに分類すると以下の3 つに絞ることがで きました。 コードの再利用性について(10) コードの可読性について(20) パフォーマンスについて(3) Press Space for next page appliv(71) ピタッとROAS 集計システム(152) 計測システム（37 ） 管理画面（32 ） 合計（293 ）
6. 3. レビューを振り返る 分類した各項目で代表的なレビューを選びどのようなことを当時考え、レビューをもらってきたのか振り返っ てみたいと思います。 特に自身が今後開発・改善していくときにはこれから出てくる点を意識して業務に取り組んで行きたいと思い ます。 Press Space for next page
7. 3.1: コードの再利用性について 事例：異なるクラスで、同じような処理を行なっている Press Space for next page コードの再利用性を意識することは大切です。コードが冗長にならないよう同じような処理でラッピングする ことで、誰でも手を加えやすいコードを保つことができます。 class hoogeRepository { public function updateConversions($update_data) { // コンバージョン数を計算して、DB にupsert する結構長い処理 } } //-----------------------------------------// class hoge2Repository { public function updateConversions($update_data) { // コンバージョン数を計算して、DB にupsert する結構長い処理 } }
8. レビュー 親のクラスを継承するとで、異なるRepository 間で同じような処理を書くことを防ぐことができます Press Space for next page ta9to on 13 Apr Outdated 内容おんなじなので一個上に適当なクラス置いてそこに実装置いて継承するかtrait に切り出すとかしたほうがいいかも
9. 改善例 class hoogeRepository { public function __construct(hoge $eloquent) { $this->eloquent = $eloquent; } public function updateConversions($update_data) { // コンバージョン数を計算して、DB にupsert する結構長い処理 } } //-----------------------------------------// class hoge2Repository extends hoogeRepository { public function __construct(hgoe2 $eloquent) { $this->eloquent = $eloquent; } } //-----------------------------------------// // 呼び出し元 $this->hoogeRepository->updateConversions($update_data); $this->hoge2Repository->updateConversions($update_data);
10. コードの可読性について 事例：データベースで管理している値をハードコードで指定して処理をしている この三項演算子はOS の判定で、os_id が１ならIOS でそれ以外ならandroid 。のように処理を分けている 確かにこれでは、何にもわからん レビュー Press Space for next page コードの可読性はチーム開発においてとても重要です。コードが冗長で可読性が低くなると、今後私や私以外 の人がこのコードを見た時、理解するのに時間を要することになります。また、一目で意味を理解できないの でバグを誘発することも考えられます。 $data['campaign'] = $App->os_id == 1 ? ($ios になんかする) : ($android になんかする); $App->os_id == 1 ？？？ マジックナンバーValueObject をつくって$App->isIos() とかで判定したい
11. 改善例 私は１がどのような意味を持つのかを把握していますが、レビューしている人がそれを把握していることを理 解していませんでした。可読性の指摘は他にもありましたが、これは特に可読性が改善された例だと思いま す。 class Os extends ValueObject { public function isIos():bool { return $this->value === self::IOS_ID; } public function isAndroid():bool { return $this->value === self::ANDROID_ID; } } //-----------------------------------------// // 呼び出し元 if ($Os->isIos()) { hgoehgoe } elseif ($Os->isAndroid()) { hogehgoe }
12. パフォーマンスについて 事例：$columns の全行の処理で都度getAdNetworkById というDB アクセスが発生している AdNetworkId が０以上ならばDB にアクセスしてID に紐づくAdnetwork 情報を取得してくるような処理です。 これだとmap の中の全行でgetAdNetworkById が処理されDB アクセスが伴います。 Press Space for next page 実装時に、常にパフォーマンスについて意識して実装することは大切です。 実際にこのレビューをもらってから、DB アクセス時などはここに書くことが最適なのか？と常に考えるよう になりました public static function hoge(int $data_type, array &$columns) { switch($data_type) { // 省略 return function ($row, $key) use (&$columns) { try { // 省略 $map_row['Ad Network NAME'] = $map_row['Ad Network ID'] !== 0 ? AdNetwork::getAdNetworkById($map
13. レビュー 指摘のように関数呼び出し直後のswitch 文の前で、事前に全てのAdNetwork を連想配列で取得しておくこと で、全行での処理では、連想配列からID で検索するだけですみますね Press Space for next page ここでad_network テーブルに問い合わせると、CSV を１行処理するごとにDB アクセスが発生してパフォーマンス的に宜しくないかと！事前にad_network の全 先日、この案件を対応しようと調査したときは以下のように考えていました。ご参考まで！ 1. hoge の呼び出し直後にad_network の全データを連想配列として取得 - $ad_networks = self::getAdNetworkInfo(); 2. CSV を処理するところではID をキーに連想配列からADNW 名を取得 - $map_row['Ad Network Name'] = self::getAdNetworkName($map_row['Ad Network ID'], $ad_networks);
14. 改善例 関数呼び出し直後に、$ad_networks で全てのデータをキャッシュしておくことで、DB アクセスを１回にして います。 DB に処理をする実装を入れるときは、よくこのレビューを思い出して、どこで取りに行くのがベストか意識す るようになりました Press Space for next page public static function hoge(int $data_type, array &$columns) { $ad_networks = AdNetwork::getAllAdNetworks(); switch($data_type) { // 省略 return function ($row, $key) use (&$columns) { try { // 省略 $map_row['Ad Network Name'] = $map_row['Ad Network ID'] !== 0 ? $ad_networks[$map_row['Ad Networ
15. パフォーマンスへの意識２ 事例：どうせpluck するのに、pluck の前でfilter している これはS3 のCSV にアクセスして、CSV のID 列のみ返すような処理をしていて、filter の目的は空のものを弾くた めです これだと最終的なものはID 列だけなのに、filter で全行回しています Press Space for next page これもパフォーマンス周りの事例です 実際広告事業ではCSV を取得してきて、全行回すみたいな処理をすることは多いので、ここへの意識はとても 大事だなと思っています return Csv::s3PathToArray($path)->filter(function (array $row){ return $row['ID']; })->pluck('ID')->toArray();
16. レビュー 確かに！！ 改善例 先にID 列のみpluck しておくことで、filter のオーバーヘッドも減り、コードも見やすくなりました 実装しているときは、どんどん手が動いてしまい、このような細かい部分の改善に実装者は気づきにくいと思 います。 Press Space for next page filter() の意図は ID が空のものを弾く意図ですよね？であれば pluck() してから空を弾いたほうがオーバーヘッドが少ないかなと。 Csv::s3PathToArray($path)->pluck('ID')->filter()->toArray();
17. 過去のレビューを振り返ってみてどうだったか こんなレビューもらってた！なんていう発見がたくさんありま した。 自分のコードをより良くするためには、新しいことを学ぶこと に加えて、過去のレビューを定期的に見返すこともとても効果 的だと思いました