JaSST ’19 Niigata

実践コードレビュー
Code Review in Practiceウォーターセル株式会社
松井 正志

1

はじめに

2

はじめに
• 予稿集と発表資料でスライドが大きく異なります。
• 大筋の内容は一緒ですのでご安心ください。
• 発表スライドは後日Webで公開予定です。
• オフレコの内容はないのでブログ・SNSなどへのアップはOKです。

3

自己紹介

4

松井正志

ウ ォ ータ ー セ ル 株式 会社
フ ロ ント エ ン ド エン ジニ ア!5

JS
JavaScript!6

!7

ウォーターセル株式会社• 新潟市にある農業ICTをやっている会社• 主な事業は営農支援サービス「アグリノート」の開発・運営

8https://www.agri-note.jp/!9https://www.agri-note.jp/!10コードレビューとは11コードレビュー（英: Code review）
は、ソフトウェア開発工程で見過ごされた誤りを検出・修正することを目的としてソースコードの体系的な検査（査読）を行う作業のこと。
コードレビュー - wikipedia
https://ja.wikipedia.org/wiki/コードレビュー
12コードレビューとは• ソースコードに対するレビュー• 近年ではgithubなどの台頭により、メインブランチのマージ前に行われることが多い

13

メリット
• テスト工程の前に不具合を発見できる• 可読性などのコードの品質が向上する• 仕様や実装の属人化が防止できる

14

デメリット• レビューに時間がかかる• 本質的な部分がレビューされているとは限らない• 形骸化してしまう可能性がある15プルリクエスト16

https://git-scm.com/downloads/logos
17

Git• バージョン管理システムの一つ• 分散型であることが特徴• Linuxカーネルのソースコード管理のためにリーナス・トーバルズによって開発された

18

https://www.shopify.com/partners/blog/git-guide19https://github.com/logos20

21Github• Gitのリポジトリを管理するサービス• リポジトリを管理するだけでなく、Web上でのコードレビューなどの機能も持つ• 多くのOSSのプロジェクトがホスティングされている
22

Pull Request

23

24

Pull Request• 作業ブランチをメインブランチにマージする

依頼• Git自体の機能ではなくGithubなどの機能• Gitlabの場合はMerge Requestと呼ぶが基本的には同じ
25

Case Study!26

事例紹介• ウォーターセルの場合• 基本的にすべてのコードはレビューを通す• レビューワーは一人だったり複数だったりす

る27

開発の流れ

設
計実
装

28

テス
ト

実装の流れ

29

マーー

実
装レビ
ュ

ジ

プルリクエスト• 大体はチームの誰かを指名する• 実装した機能の仕様や使っている技術がわかる人を指名する• 場合によっては複数人指名する場合もある

30

31

32

33

34

よく使う略語：LGTM

35

LGTM• Looks Good To Me• 日本語に訳すと「自分的によさげ」• レビューして特に問題がなかったときなどに使う

36使っているツール37使っているツール• Gitlab• Gitlab CI/CD• Chatwork38https://about.gitlab.com/press/press-kit/
39Gitlab• Githubライクな機能を持ったGitリポジトリマ

ネージャー• クラウドでもオンプレでも使える• CE版とEE版がある

40

https://about.gitlab.com/press/press-kit/
41https://about.gitlab.com/press/press-kit/
42https://about.gitlab.com/press/press-kit/
43Gitlab CI/CD• GitlabのCI/CDの仕組み• ジョブの実行はGitLab Runnerで実行される

44

CIとは
• 継続的インテグレーションの略• リポジトリにプッシュするごとにテストやビルドなどを自動で実行する• テストやビルドに成功しないとプルリクエス

トをマージできない45

CDとは (1)
• 継続的デリバリーの略• メインブランチへの変更は、テストに成功したら自動で検証環境などにデプロイされる46

CDとは (2)
• 継続的デプロイの略• メインブランチへの変更は、テストに成功したら自動で本番環境に即デプロイする47

弊社の場合• 継続的インテグレーションはやってる• 継続的デリバリー・デプロイはやっていない• テスト環境へのデプロイは半自動で行なっている48

https://go.chatwork.com/ja/download/cw_logo.html
49https://blog-ja.chatwork.com/2017/06/blog-post.html
50

Chatwork• 開発はもちろん、あらゆる業務で使用している• API経由で各種通知を流している• レビュー依頼、コメント、CIの失敗、etc…

51よく使う略語：WIP

52

WIP
• Work In Progressの略• 日本語に訳すと「作業中」• まだ実装途中でマージしないで欲しい時にタイトルにつけたりする

53自動化しよう54

自動化しよう• 通知• コードフォーマット
• CI• 確認環境の構築

55

通知• 何にしてもいちいち確認するのは面倒• 節目節目のイベントで通知を飛ばす• 飛ばしすぎに注意

56

CI (再掲)• Continuous Integration• ビルドやテストを自動化することで、それらが通らないコードの混入を防ぐことができる

57

コードフォーマット
• もっとも自動化しやすいレビュー項目の一つ• 自動化することでコーディング時のルールも明確化される• 創意工夫のこもったコードは諦める

58

確認環境の構築• 手元で毎回動作確認をするのは大変• 動作確認がすぐにできると便利• ビルド結果がすぐダウンロードできるとか、ワンクリックでWebサーバーが立ち上がるとか59

レビューされる側60

本文を書こう• 内容が自明な場合は省略してもいい• チケットやチャットへのリンクを貼るのもGood

61

範囲を明確に• やったこと、やってないことを書くと、レビューする側にとってはありがたい62スクショを貼ろう• 物によってはいちいち動作確認せずにすむ• 何より何をしたかの理解が早い

63

テストを書こう• コードが何をしているかが自明になる• 実装した人がどこに気をつけてコードを書いたかがわかる• 実装コードよりテストコードのレビューの方が力を入れることも

64適切な粒度で• 原則、変更内容は一つ。大きすぎる時に分けたり、細かすぎる時にまとめたり• 粒度が大きすぎるとレビューがつらい• 粒度が小さすぎるとやりとりのコストが高い

65

66

67

よく使う略語：SSIA

68

SSIA
• Subject Says It All• 日本語に訳すと「タイトルが全てを表している」• タイトル本文に特に書くことがない場合に69

レビューする側

70

自分のことは棚に上げる• 自分ができてないから…と遠慮すると何も言えない• レビューするときは自分のことは全て棚に上げてレビューする• レビューされるときは頑張る71忖度しない• 理由が明確でないなら確認すべき• 何か理由があってこうしているのだろうと想像してはいけない• 案外単なる凡ミスのことが多い72

遠慮せず聞く• 疑問があったら遠慮せず聞く• あなたがわからないことはみんなもわからない73

緩急をつける• 全てをみっちりやると力尽きる• 見るところは見る、眺める程度の場所はさっと眺める

74

指摘は具体的に• ふわっとした指摘はお互いに疲弊するだけ• 実装例を書いてしまってもいい• どうしようもなくなったらMRにMRを出す75

かたくなりすぎない• とかくレビューは殺伐としがち• あまりかたくならないように柔らかい口調で• 絵文字を使うのもGood 👍

76

まとめ

77

まとめ
• コードレビューには、メリットとデメリットがある• コードレビューは自動化により効率化できる• レビューをする側・される側の両方にそれぞれコツがある

78