ざっくり背景説明
会社の若手（二年目らしい、部署は違うがロケーションは近い）のコードレビューをすることになりました。
なぜいきなり私のところに来たかといえば、昔ちょっとPowershellをかじっていて、それが風のうわさで耳に入ったから、という感じでした。まぁ有識者の一人としてちょっとみてやってくれや、みたいな感じです。
ちなみにそのレビュイー（レビュー受ける人ですね）のいる場所は、完全に開発を外出しにしていた弊害で課題が多発してしまったので、今ちょっとずつ内製化に切り替えているところ、その一環としてその若手がコードを書くことになったみたいです。
これまでの文脈からわかると思いますが、対象言語は Windows PowerShell です（とはいえPowerShellらしい話はほとんどしません）
また、他部署を巻き込む大規模なレビューをそれなりの大きさの会社がやるときのあるあるで考えてもらいたいんですが、まぁコード自体はすでに出来上がっていてのビッグバンレビューですよね。それは仕方ない。
コードを見て気になったこと
残念なことですが、品質として高いものではありませんでした。とはいえ、最終レビューに持ってくるくらいですから、機能的な不足やひと目見ただけでわかるバグはない感じでした。（実際、二年目の子が書いたんだとすればそれだけでも立派なもんです。）
問題に感じたのはその書き方です。気になったのは大きく以下の３点。
- 意味が明確でない名前付け
- 無意味なコメント
- プログラミング言語の特性を無視した書き方
それぞれ、かんたんに解説します。
１． 意味が明確でない名前付け
まず気になったのが名前付け。その中でも一時変数の名前付けです。
例えばこういう名前。
$date = Get-Date -Format "yyyyMMddHHmmss"
$wk_file = $date + ".wk"
まぁ、わかります。日付でファイル名作ってそれを一時ファイル（workingファイル？）にしているんでしょう。
でもやっぱり date はダメですよ。 せめて current_date でしょうし、むしろ削るなら date のほうですよね。狭いスコープならいいのでしょうが、ちょっとでもスコープ長くなるとバグを生む元になります。
あと省略形も良くない。working を wk と省略するのはおそらく、そこまで一般的ではないはず。ちなみに私は一瞬 weak のことで、弱参照でも使っているのかと身構えました。これは少数派でしょうけど、省略名は読み手に少なからずの負荷を与えることを、書き手はもっと意識するべきです。
２． 無意味なコメント
こういうのです。
# get credential
function get_credential
こういうコメントがほぼ全関数に打ってありました。追加の情報量が、ありませんよね。
これだったら書かないほうがマシです。なぜなら、コメントを書くというのはメンテナンス箇所を倍にしているからです。
ちゃんと関数分割で処理ごとに分けているのだから、素直に関数名で表現すればいいのです。
そうすることで、無駄なメンテナンスコストが避けられます。
３． プログラミング言語の特性を無視した書き方
PowerShellと聞いていたので、最初の数行でまずは面食らいました。
#################################################
# Name
# Credential Management Tool
#
# Usage
# CredentialTool.ps1 ARG1 ARG２ ARG3
#
# Parameters
#
# （中略）
#
#################################################
感覚でしかないですが、古いコードの香りがします。うーむ。。。
さて、PowerShellにも、当然Docコメントはあります。
そもそもブロックコメントで書くことになってますし、書き方も違います。当然Docコメントですから、Helpの自動生成くらいはやってくれます。
もっと言えば、PowerShellにはデフォルトで -LoginId my_id のように、パラメータ名を指定するやり方もあります。
というわけで、このへんから始まって一事が万事、PowerShellの機能を使っていないのです。
ほとんどCMDで、ところどころにPowerShellで追加された機能を使っている、、、そんな感じです。
もちろん社内での技術レベルによって何らかの制限をかけたりする場合はあるのだとは思いますが、そのへんの議論がされていたわけではなく、ただ単に前コードの踏襲という形で、この形になったようです。
だとすれば、指摘する他ありません。
指摘するときに気をつけたこと
とはいえ、じゃあ好き勝手にこの辺の意見を投げつけにいったら、他所の部署の人からすると天災のように扱われるのがオチです（そもそも開発終盤ですからね）。
というわけで、ちょっとでも意見を取り入れてもらえるよう、以下のような工夫をしました。
- レビュー指摘に段階をもたせる
- 指摘は極力丁寧にWHYが分かるように、サンプルコードやドキュメントを添える
- 良い点もしっかりレビュー指摘として上げる
一個ずつ、ちょっと掘り下げます。
レビュー指摘に段階をもたせる
まず、最初にこれだけはやろうと思っていました。
Githubなどのコードレビューでよくある、あれです。今回は以下の３つを使いました。
- nits: 軽微な指摘事項
- IMO: 個人的には気になる、直したほうがいいと思うこと（In My Opninion）
- MUST: 必ず直してほしいこと。今回でいうと、ほぼプログラムエラーを引き起こしそうな境界値処理や、今書かれているルールに従ったとしても整合性が取れない命名規則など
結果的には、上記の指摘はすべてIMOにしています。実際プロセスから行っても直せないのですよ。
なので、完全に知識の伝達が主目的ですね。
とはいえ、この区分けは一緒にレビューをしたちょっと年配のエンジニアの人たちに好評でした。「いつもレビューの指摘”nits”なことに時間使ってるよな（ガハハ）」な感じで、こういう文化があることに興味を持ってもらったよう。
指摘は極力丁寧にWHYが分かるように、サンプルコードやドキュメントを添える
知識の伝達が目的ですから、当然論拠を示し、納得してもらい、自分のものに少しでもしてもらわないといけません。
前章で示したことを、さらに噛み砕いて説明し、言語仕様へのリンクを張り、他の言語での考え方を紹介し、さらにもっと包括的な知識がほしいならばと、「リーダブルコード」 や現場で役立つシステム設計の原則などの紹介もしました。
実際、これからたくさん書いていくコードはきっとPowerShellではないので、PowerShellの文法よりも、「なぜPowerShellという言語だとこう書くのか、他の言語だとどうなのか」を伝えました。
良い点もしっかりレビュー指摘として上げる
今回のレビューイは、まだまだコードを書きたてで、人のコードを真似ながら書いてます。そんなとき、真似しているコードの何が良くて、何が悪いかとか、わかんないですよね。
というわけで、上で書いた３つの指摘項目に加えて、[GOOD]という軸でもレビューを書きました。
- [GOOD] ここを関数に切り出しているのはわかりやすいです
- [GOOD] わかりやすい名前付けでいいいですね
こんな感じで、割と些細なことでも、いいなと思った箇所にはレビューを書き込みました。
これもベテランの方に好評だったんですよね。よっぽど今までギスギスしたレビューをしてきたんだろうか、、、などと思ったりしました（笑）
レビューしてみてどうだったか
久しぶりに時間を撮ってやってみて、改めてですが自分の考えを言語化して整理できたのは大きかったと思います。
最後の方はレビューハイになっていて、深夜までずーっとコメント書いていましたしね（それはそれでよくない）
改めてコードを見て、指摘を出すというのは良い訓練です。それはレビュアーとレビュイーの間でもそうですし、レビュアー同士もそうだと思います（他所の部署のベテランの方と意気投合できました）
コードは共通言語なので、意外とレビューはコミュニケーションの鎹になってくれるのかもしれません。
さて、、つぎは外国籍の方ともちょっとやってみるか。