Upgrade to Pro — share decks privately, control downloads, hide ads and more …

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

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

Kairi Yasumatsu

June 30, 2021
Tweet

More Decks by Kairi Yasumatsu

Other Decks in Programming

Transcript

  1. Who I am ? 安松 海里 ナイル株式会社: メディアテクノロジー事業部 (20 卒入社) (インターン:2019 夏くらい〜) 💻

    職種: エンジニア  Appliv ( インターン) ピタッとROAS  (入社〜) 🧑‍💻 趣味: 個人開発・プロ野球・NBA ・海外サッカーとか  ※ 今回のLT では、マークダウンに慣れている訳でもないのに、slidev を使用しております Press Space for next page
  2. 目次 1. コードレビューについて 2. もらったコードレビューを分類した 3. レビューを振り返る 3.1: コードの再利用性について 3.2:

    コードの可読性について 3.3: パフォーマンスへの意識 Press Space for next page 新卒1年目でもらったコードレビューを振り返って血肉とする
  3. 1. コードレビューについて チーム内でタスクの洗い出し 要件定義 実装 GitHub でPR (プルリクエスト)を投げる レビューを受ける OK

    が出るまで修正する LGTM! 今回はレビューを分類し、その中で印象に残っていたり、重要だなと思ったレビューをピックアップしました Press Space for next page 今の業務ではGitHub を用いたコードレビューをしています。おおまかな開発フローは以下のような感じです。
  4. 2. もらったコードレビューを分類した 今までに私がGitHub 上で出したPR の数はインターン期間を含めた約2年間で、合計292 個でした。その中か ら、直近のピタッとROAS での実装で、特に多かったレビューを大まかに分類すると以下の3 つに絞ることがで きました。

    コードの再利用性について(10) コードの可読性について(20) パフォーマンスについて(3) Press Space for next page appliv(71) ピタッとROAS 集計システム(152) 計測システム(37 ) 管理画面(32 ) 合計(293 )
  5. 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 する結構長い処理 } }
  6. レビュー 親のクラスを継承するとで、異なるRepository 間で同じような処理を書くことを防ぐことができます Press Space for next page ta9to on

    13 Apr Outdated 内容おんなじなので一個上に適当なクラス置いてそこに実装置いて継承するかtrait に切り出すとかしたほうがいいかも
  7. 改善例 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);
  8. コードの可読性について 事例:データベースで管理している値をハードコードで指定して処理をしている この三項演算子はOS の判定で、os_id が1ならIOS でそれ以外ならandroid 。のように処理を分けている 確かにこれでは、何にもわからん レビュー Press

    Space for next page コードの可読性はチーム開発においてとても重要です。コードが冗長で可読性が低くなると、今後私や私以外 の人がこのコードを見た時、理解するのに時間を要することになります。また、一目で意味を理解できないの でバグを誘発することも考えられます。 $data['campaign'] = $App->os_id == 1 ? ($ios になんかする) : ($android になんかする); $App->os_id == 1 ??? マジックナンバーValueObject をつくって$App->isIos() とかで判定したい
  9. 改善例 私は1がどのような意味を持つのかを把握していますが、レビューしている人がそれを把握していることを理 解していませんでした。可読性の指摘は他にもありましたが、これは特に可読性が改善された例だと思いま す。 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 }
  10. パフォーマンスについて 事例:$columns の全行の処理で都度getAdNetworkById というDB アクセスが発生している AdNetworkId が0以上ならば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
  11. レビュー 指摘のように関数呼び出し直後のswitch 文の前で、事前に全てのAdNetwork を連想配列で取得しておくこと で、全行での処理では、連想配列からID で検索するだけですみますね Press Space for next

    page ここでad_network テーブルに問い合わせると、CSV を1行処理するごとに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);
  12. 改善例 関数呼び出し直後に、$ad_networks で全てのデータをキャッシュしておくことで、DB アクセスを1回にして います。 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
  13. パフォーマンスへの意識2 事例:どうせ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();
  14. レビュー 確かに!! 改善例 先にID 列のみpluck しておくことで、filter のオーバーヘッドも減り、コードも見やすくなりました 実装しているときは、どんどん手が動いてしまい、このような細かい部分の改善に実装者は気づきにくいと思 います。 Press

    Space for next page filter() の意図は ID が空のものを弾く意図ですよね?であれば pluck() してから空を弾いたほうがオーバーヘッドが少ないかなと。 Csv::s3PathToArray($path)->pluck('ID')->filter()->toArray();