Hatena::ブログ(Diary)

naoyaのはてなダイアリー

March 13, 2014

些末なコードレビュー

朝起きて布団から出るのがつらいので、HBFav をつらつらと眺めていた。

あるサービスの JavaScript が重いとか、そのコードが難読化されてないとか、担当者とおぼしき人間が書いたコメントがそのまま残ってるから消しましょうよとか、そんなことが書かれていた。JavaScript が重い、という話は結局そのサービスの JavaScript が重かったのではなく、ユーザーが自分で導入した広告が重いというだけの話だった。

コードが難読化されていない、趣味の製品ではなく会社の製品なのでコメントそのまま残ってるから消しましょう・・・実にくだらない。

ところで話は変わってコードレビューについて。

コードレビューに慣れないチームが、何の考えもナシにコードレビューを始めるととにかく気になったこと大小様々な指摘が行われることになる。一見、いろいろな指摘が出て議論が活発になっているように見えるが、だいたい議論が紛糾しているのは「コードのインデント幅が違う」とか「return が省略されてる。俺は return があったほうが好み」とか「その場合は字下げをした方が綺麗にみえるんでは」とか、そんな些末なことばかりである、ということが多い。必ず一度は通る道である。

そんなことを延々議論していたって、はっきり言って何の意味もない。何の意味もない、は言い過ぎにしても、そんなところを改善したところで実質的な品質は何ひとつ上がらないわけだし、どうしても揃えたいなら lint ツールか何かで機械的にチェックすればよいことであって人間がやることではない。

やらなければいけないのは、「その設計は拡張に対して開いていないから開くべき」とか「これではエッジケースが想定されていないからこういう不具合につながるのでは」とか「そのテストでは後日見返したときに第三者が要求仕様を解釈しづらい」とかそういう指摘である。これらはちゃんとコードを読んで、コードの構造を把握して、そこに書かれているコード以外のシステムの全体感が頭に入っていて、初めてできる指摘である。当然、それをするにはレビュワーにも知識とスキル、そして真摯な態度が要求される。

JavaScript にコメントが残っているからダメだ、なんてのはインデント幅が2じゃない、4にしろ、と言っているようなものである。自転車置き場の議論を読むべき。難読化は、しなければいけないというものではない。仮に難読化ではなくソース圧縮だって速度的にそこが律速ならするべきだが、多少の JavaScript の文字数を減らしたところで、他のファイルとのサイズあるいは gzip 圧縮との相対感からいくとその効果はハナクソみたいなものであることがほとんどだ。

インデントがとか、コメントがみたいな指摘をしているのを目にすると、くだらない、と妙に腹が立つ。

なんで腹が立つのだろうか。

それは過去の自分がまさにそれそのものだったからだ。人間、他人に腹を立てるときは、そいつがあまりにも自分に似ているか、そいつが過去の自分・・・自己嫌悪の対象だった自分と重なってみえるからだ。自分の実力のなさをごまかすため、自分を大きくみせたいがために、些細な指摘をする。そのコメントは必要ないとか、if 文の条件の書き方がなってないとか、そこの括弧は省略できる、とか。そしてそんなことで優位に立ったと、自分を大きく見せられたと、思いこんでいる。やがて時が経ちいろいろ経験を積んだ頃になって、当時周りの先輩たちは、お里の知れてる自分を生暖かく見守ってくれていただけであることに気づくのである。

追記

自分は元のエントリを明確に批判しているが、それが原因で話があらぬ方向に行くのを避けようと思い敢えてどのエントリかは書かなかったが、この記事が拡散される中、それこそ要らない誤解を招きそうなので追記する。

そしてはてなブックマークのコメントへの反応その他を見ていると「コメントが残っていたことではなく、コメントの内容に問題があったのが問題なのでは」と指摘があった。自分は、上記エントリを読んだがコメントの「内容」のどこに問題があるのか全く理解できなかった。

パスワードなど、何かユーザーやこのサービスに不利益になることがコメントに書かれているのか? 古いコメントが残っていてコードの動作と反するような記述になっているのか? コメントが原因で、そのほか悪影響を及ぼすようなことはあるのか。見た限り、そんなことはなさそうだ。もしそのようなコメントであれば、それは当然コードレビューでも指摘するべき。パスワードが云々のような本当の意味で致命的なものなら、第三者がインターネット上などで指摘することは重要だと思う。

一方、このエントリに書かれているのは「恥ずかしいコメントを書いちゃだめ」「趣味ではなく会社の製品なんだからコメント消しましょう」「コピペと言わずに共通化」と言いましょうというような指摘で、いずれもどうしてそうしなければいけないのか自分にはわからない。むしろ「些細なエラーなので無視する」というコメントなどは現在 console に出ているエラーは無視しても構わないと開発者が判断しているというヒントになるのであり、そのコメントがあることが有益であるとすら感じた。

はてなブックマークの方では「コメントの内容が致命的にひどい」とあるが、本当に致命的にひどいのであれば、何か致命的な問題が起こるだろう。そのようなことが起こっているのか。また、一企業がコメントで意図をあけすけにするのは微妙だから、というのは全く賛同できない。なぜ隠す必要がないようなことまで、無難な方に倒して、隠す必要があるのか。

コードレビュー中にコーディング規約に類することには意味がない、と書いた。一方「意味がないとも言わないが」とも書いた。意図としては、本来設計そのほかもっとレビューしなければいけない項目があるのに、規約についてばかり議論していてもしょうがない、それは木を見て森を見ずだということを述べている。当然、初学者の新人がインデントもコメントもぐちゃぐちゃなコードを書いてきたらそれはレビューで指摘するべきだし、レビュイー本人の成長にもつながる。それは程度の問題である。

本記事を読む各人が置かれている環境・・・まわりに規約を全く守れない開発者がいる、規約すらない、などいろいろな状況があるとは思う。その自分の環境に重ねていろいろ想像されることは各人の自由だと思うので、そのことについては特に思うことはない。すくなくとも、自分の場合も、自分の周囲の開発者のスキルや傾向などを想定・前提として書いている。

自分が暗黙的に想像としているサービスは自社開発のWebサービスであり、顧客に納品するミッションクリティカルな業務アプリケーション、ではない。元になったエントリもまた、自社開発のWebサービスについて論じている。(この一文は対象ドメインが違うと作法も異なるであろうという意図で書いたが「ミッションクリティカル」は余計な要素だと感じたので del とする。決して自社開発Webサービスは品質を犠牲にしてよいと言っているわけではない)

追記2

上記2エントリのうち、後者 id:hevohevo さんに関しては、コメントの内容については問題がなく肯定的であると論じています。それに関して、上記2エントリどちらもが批判的なエントリを書かれていると、本エントリの読者の方に受け取られかねない記述をしてしまったことをお詫びいたします。

id:hevohevo さんのエントリで私が賛同できない部分は、「会社のプロダクトなのでコメントは隠そう」「miniftyすべき」という点です。その賛同できない理由については前述の通りです。また、私は賛同できないが id:hevohevo さんにはコメントを隠す、minify するということには理由があろうということは追記されていました。(いずれにせよ、私はそれにはあまり賛同できません。minify すべきか、コメントを隠すべきかはその開発チームのポリシーや優先度で決まることであり、どちらが良いかという一般論ではないと思っています。)

ご迷惑おかけしました。