背景

サービス責任者はあくまでサービス全体を見ているため、特定の技術のみに注目した深掘りがしづらい状況となりがちです。
気づかないうちにコードの品質が落ちたりする部分が発生し易くなります。
また複数のチームで同じことを行ったりすることがあり、組織全体では効率が悪いケースもありました。

サービス責任者の負担を少しでも減らすため、またスペシャリストが取り組むべき課題を発見するために棚卸しを実施することにしました。

目的

目的は以下の3つになります。

チームの技術力把握
開発でどんなコードを書いているか、レビューでどんな議論をしているのか
技術に対する提案内容の模索
開発チームに対して品質や効率化が実施できる提案はできないか
チームから吸収できる技術の把握
開発チームが開発を通して得た知見や挑戦した内容で他チームに展開すべきものは無いのか

実施の流れ

f:id:AdwaysEngineerBlog:20201009163129p:plain

現状はScala、JavaScript(Vue,TypeScript)、Ansibleの3つで棚卸し作業を行なっています。

（事前準備）直近（2~3ヶ月）にマージされた一覧を取得
（45~60分）コードの棚卸しの実施
黙々コードレビュー
全体を通して活動すべき内容の洗い出し
- （15分）コードレビューの結果を黙々読んで感想をまとめる
- （20~30分）議論して技術毎のスペシャリストとして実施する内容をまとめる

コードレビューでのラベルの種類

実際の開発文化では「MUST」「IMO」などを活用していますが、棚卸し用にラベルは変更しています。
MUSTは必ず修正すべきコードであると言ったネガティブな印象があったので、Goodなどのポジティブなラベルをメインにしています。
第3者からのレビューになるのでその開発当時の状況を把握しないまま、修正すべきものと決めつけるのは良くないと判断しました。

種類	説明
best	素晴らしい、他のチームにも展開する様に動く
good	良いコードだ
ask	質問、疑問
suggestion	提案

実際のレビューでのコメント

色々な言語を混ぜてます。

[best]
scalafmtは展開を意識して、突き詰めても良いかも
境界を意識した制限をかけているのは良いね。これは当たり前にしたい
クリックのイベントを解析する仕組みをmixinで共通化、汎用的に使いまわしている。他のプロジェクトでも活用してみたい
code deployがAWSのデプロイのスタンダートになっても良いかもね
rundeckのyml管理をジョブ毎でAnsibleでやれているのは良いね

[good]
境界を意識した制限をかけているのは良いね
ValueObjectとして綺麗なコードだ不具合時にしっかりテストを追加して対応しているのは素晴らしい
不具合時にしっかりテストを追加して対応しているのは素晴らしい
@throwsのアノテーションを利用している。これはどんな運用だろうかきになるね
constの活用がされている。lintツールのおかげかな
ABTestの実装をwebpackで切り替えられる仕組みを採用していて良いと思った
Ansibleも整備されパスワードとかは暗号化してるし、コード化で設定がよく分かるし綺麗になってきたなと思う内容だった
ansible-vaultによる暗号化が浸透している
spotインスタンス使用は積極的進めたい

[ask]
specs2での「there was no...」は初めて見た。どんな使い方にしているか知りたい
XXXListEntityとXXXListResultEntityの違いは何だろう？ほとんど一緒？
このテストの書き方は辛そうな気がするけど、大丈夫か心配。もうちょい楽なJSONのテストコードの書き方があったほうが良いな
環境毎に値が違うからconfig管理していたんじゃなかったっけ？

[suggestion]
foldReftを使った書き方をすればvarは減らせそう
コレクションオブジェクトを使うとスッキリしそう
無理にネストせずにmapの部分とか外出ししたほうが可読性は高いと思う
ドメインの処理に外部起因のTryを持ち込ませないようにしたい
単体テストが欲しいと思っちゃった
この記法が出てくるがどちらかにあわせてもいいかも

棚卸しを経て実施したこと

技術調査系はもちろん、リファクタリングを実施して新しい書き方を教える教育系の内容も棚卸し後に実施しました。
去年くらいから四半期毎で実施していますが、少しずつ実績が出来ています。

Ansible MoleculeでEC2を活用したサーバグループ単位のテストの模索
Scala CleanArchitectureについて自分なりに整理して実装してみた
Scala jsonライブラリ調査（circe）
Scala varとループを使ったコードのリファクタリング
JavaScript ESLint棚卸し
JavaScript ua-parser-jsを活用したUA処理のリファクタリング

blog.engineer.adways.net

感想

初めは仕様を知らないため、うまくレビューできるかは不安でした。
実施してみると仕様を知らないことは問題ではなく、逆に知らないからこそ
コードに対して技術観点のみに絞ったレビューが実施できました。
また、開発時に行なったレビューコメントなどからチームの考え方や状況（結論に至るまでの流れ）を追えるので面白いです。
技術的な課題に対しては開発メンバーにヒアリングするよりサクッとコードを見た方が
そのチームの状況や技術力は把握できるんだなと思いました。

もし、他チームへの介入に悩んでいる人がいたらまず直近マージされたコードやレビューコメントを見ることをお勧めします！！