コードレビューって意味あるの ? 89
ストーリー by reo
どうなんでしょ 部門より
どうなんでしょ 部門より
pinbou 曰く、
本家 /. の記事 ``Are Code Reviews Worth It ?'' より。悩めるマネージャからのご相談。
私は開発マネージャです。先日、上司と私はコードレビューの価値について議論しました。私の部署ではプログラマ主導でコードレビューとデザインレビューをやっています。やってみてわかったことは、コードレビューはやたら時間がかかる割に、良質な UI レベルでのテストよりも分かることが少ないということです。一方、デザインレビューの効用は絶大です。そもそも私たちの書いているコードはデスクトップ向けの、非クリティカルな用途のものなので、私は上司に、コードを精査するのはあまり意味がないのではないかと尋ねてみたのです。Slashdot の皆さんはどう思われますか。
もちろん意味がありますとも (スコア:4, すばらしい洞察)
「こういうコードが恥ずかしいコードである」
という価値観について、上級技術者間で意識統一がなされていればね。
ようするにコードレビューと言うのは、大学の研究室で言う輪講とかと同じなんです。
コードをよりよいものにする、と言うのも目的の一つですが、コードを組んだ人のレベルアップを図る、という目的もある。
十分な人数の、良く判っているプログラマがいるならばペアプログラミングも良いでしょう。でもペアを組んで回れるほどレベルの高い人がいなかったら?
「教授と助教授と助手の目の前で発表させる」
しかないじゃないですか。
もちろん、この作業は「教授や助教授や助手」の時間を食います。もしあまりにも多くの時間を食うのであれば可能性は次の３つのどれか。
UIレベルのテストで問題がたくさん出てくるのですから 2 である可能性は低いですね。
1 かどうかは、レビューを受ける人とレビューをする人の人口比率を調査する必要があります。
基本的に初心者と判りきっている人に、全コードレビューに参加させるのは得策ではありません。もちろん、良いコードを読むのは重要ですが、自分が組んでいる部分と全く関係の無いコードの、しかも最初のバージョンのレビューに参加しても得られるものは少ないでしょう。それよりも「直せ」と言われた部分を直す事に時間をかけるべきです。
逆に全てのレビューに「教授・助教授・助手」レベルの人たちは参加する必要があります。実質的にコードの質を向上させているのはこの人たちですから。この人たちの時間がレビューに取られる、というならば、この人たちはコーディングをするべきではないのです。それでも足りないなら、プロジェクト全体の人数が多すぎる、と言う事です。
3 … えー、3かどうかは NDA を結んだ外部のプログラマを何人か雇って、彼らのコードレビューを受けるべきだと思います。それまで発見された問題とかなり違う問題点がいくつも発見されるようならば、 3 の状態である可能性は非常に高いです。
fjの教祖様
Re:もちろん意味がありますとも (スコア:2)
> 1. 初心者が多すぎる。そのため、「教授や助教授や助手」の時間をフルに使っても、全部など到底見切れない。コードの品質は悪いままである。
> 2. 初心者が少なすぎる。コードの品質は最初から高く、いくら見ても時間の無駄である
> 3. 「教授や助教授や助手」のレベルに到達した技術者が実はいない。なので見当違いな所を見ていたり、全員が同じ間違いを犯していていくらレビューしても品質は向上しない
結論：ほとんどの日本のIT企業においては、コードレビューは時間の無駄である。
通常は１＆３。運が良ければ１だけのこともある。
Re:もちろん意味がありますとも (スコア:1)
しかしそうすると、日本においてちゃんとしたコードがかける人たちはどこへ行ってしまったのでしょう…
fjの教祖様
あくまで道具のひとつ (スコア:3, 興味深い)
そして、コードレビュー単体として用いるのはあまり効果が無くて、
開発手法の中のその他の手法と組み合わせることで有用なこともある、といった感じ。
ただし時間が結構要るのは否めなく、その時間を取れないことが流行らない理由だとも思う。
まあコードレビューというより、コード座談会みたいなのが現代にマッチする手法かもね。
その時点でのコード・手法を皆でざっくばらんに話し合い、良い点悪い点、悩みどころなどを
気軽にぶつけあえる場を持てるような環境。どうしても個に閉じてそれっきり、ってな
雰囲気で進んでしまい、それだと進歩や改善というのが得られ難いから。
Re:あくまで道具のひとつ (スコア:1, おもしろおかしい)
話題を本筋に戻すのが大変ですわ実際
全ては今後の保守のために (スコア:3, 興味深い)
「このgetData()って何してるの？え、データの取得？データの何？HogeHoge？
だったらgetHogeHoge()って名づけようよ。んでこっちのgetData2()ってのは？」
手段編：
「転送する文字列作るのにいろいろ苦労してるみたいだね。分割したり連結したり
数値変換して、malloc()してrealloc()で伸張してまたmalloc()して（あ、メモリリーク）
……ねぇ、これってsprintf()で済むんじゃない？え、sprintf()知らない？
君、専門学校でC言語習ったって言ってたよね？」
効率編：
「このforループの中でiniファイルから値取る関数呼んでるけど、
仮に1,000回ループしたら、同じ値を取得するファイルアクセス処理を
1,000回実行することになるよ。え、サーバで動かすから速い？
そういう問題じゃねぇ！forループの直前で呼べば1回で済むだろうが！」
今日もまた、仕事のヤリ逃げを阻止する仕事が始まるお
----------科学は思考の柔軟剤
Re:全ては今後の保守のために (スコア:2, 興味深い)
趣味でやっている人は、それをやりたいという情熱なり何からの意思があるのでそういうことをきちんと考える人が多いと思うんですよね。
でもIT業界はプログラムを書いたことがない(ほとんどない)人をプログラマとして雇うこともあるんですよ。
そして#1593954の例が自社で教育中ならまだいいのですが、こういうレベルのプログラマを派遣とかしちゃう会社がないとは言い切れない業界なのです。
Re:全ては今後の保守のために (スコア:1)
残念ながら、これら３例は全て実話。しかも研修じゃなくて現場。
さらに厄介なことに、これらは機能要件は満たしている（単体・結合試験は通過する）。
こういうのを何度も見てしまうと、ソースコードには
コードレビューどころか公的監査が必要なんじゃねーか、と思ったり。
----------科学は思考の柔軟剤
Re:全ては今後の保守のために (スコア:1)
どんな業界でもプロが一番の専門家であるとは限らない。
回路は他人でも分かるが、コードは他人じゃ分からない (スコア:2)
回路は他人が設計してもある程度分かりますが、
他人の作ったコードは本当に理解できないと思う。
回路でもそれぞれの好みや設計思想があるけど、
コードは変数の名前の付け方一つで全く理解できなくなってしまう。
Re:回路は他人でも分かるが、コードは他人じゃ分からない (スコア:2, 興味深い)
>コードは変数の名前の付け方一つで全く理解できなくなってしまう。
どんなに優秀な人材だけで構成されたチームでも、個々人の個性が違うから、デザインレビューによるすり合わせは必要。
しかし、意味のある規約とそれを理解し遵守できるメンバーだけならコードレビューなんて無意味で不要。
ちゃんとした規約がなかったり、理解できなかったりしなかったりで勝手なコード書く奴がいるなら必須。
ま、そんなところかねぇ。
Re:回路は他人でも分かるが、コードは他人じゃ分からない (スコア:2, 興味深い)
とは言い切れないのでは？
・なかなか個人で（自分の目で）気づかない／見逃しやすい潜在的バグの元になるコード
・書いたコードに対するさらに見通しやすくて効率の良いコードの提案（もちろん、規約に則った記述で）
など規約の遵守外の事について、他人の目を通して分かることも多い。
また、理解し遵守できるメンバーからは外れるが新人（6月末だし、前線に出てくることもあるのでは？）も交えてやることで、知識の幅を広げさせる場としても使える。というかそう位置づけて使ってる。
Re:回路は他人でも分かるが、コードは他人じゃ分からない (スコア:2, すばらしい洞察)
これは、「だからこそコードレビューは必要なのだ」という意見ですか?
趣味で作るプログラムならともかく、仕事でプログラムするなら「他人のコードは理解できない」という(ふざけた)意見を言うほうも、それを言わせるようなプログラムを作るほうもしっかり矯正しないと駄目だよね。
Re:回路は他人でも分かるが、コードは他人じゃ分からない (スコア:2, すばらしい洞察)
業務でやってるなら、他人が読めないコードはその時点でアウト。保守できなくなる。
Re:回路は他人でも分かるが、コードは他人じゃ分からない (スコア:1)
その判らないコードを(少しでも)判るようにするための方便がコードレビューでは?
他人の目が入る可能性が高い、と言うだけでも「判りやすく(綺麗に)書こう」とか「不正コードが書き難い」と言った効用はあると思うよ。
の
Re:回路は他人でも分かるが、コードは他人じゃ分からない (スコア:1)
うーん、わかりづらい回路って、結構あると思うよ。
ただ、ほとんどの場合、CDテクニックなんで歓迎されてるだけで。
私の回路はわかりづらいと評判ですし(反省しろ
みんな幸せになればいいのに
散歩師：漫画居士柴岡秀一
http://www.toheart.to/%7Emanga
Re: (スコア:0)
Re:回路は他人でも分かるが、コードは他人じゃ分からない (スコア:2)
日本語が読み書きできれば文筆業に就けるわけではない。という意味において、変数名の命名センスに英語日本語の別が大きく関与しないという点については同意ですが、日本語で変数名を適切に導ける人間であれば、英語であっても同様とするのは、乱暴な気がします。
日本人作家が、英語でも同じように文章を書けるわけではないので。
結局、自分の語彙の中からしか選べない訳で、しっくりくる言い回しを選択できるかどうかに関しては、ネイティブであることの経験の差は大きいのだと思います。
事態は際限なく悪化する。
身も蓋もない話 (スコア:2, すばらしい洞察)
世の中には意味のあるコードレビューと、意味のないコードレビューがあって
それぞれを行っている人達が議論しても絶対にかみ合わない。
コードレビューの効果 (スコア:2, おもしろおかしい)
Re:コードレビューの効果 (スコア:1)
＞研修も兼ねて新人の女性プログラマーをレビュー担当にしたところ、
＞プログラマーが美しくてわかりやすいコードを率先して書くようになりました。
この場合、私だったら複雑で長時間説明しないと理解できないコードを書くが。不美人なんだろうか。
署名スパムがウザい？アカウント作って非表示に設定すればスッキリさ。
Re:コードレビューの効果 (スコア:1)
マジレスするなら上司の評価につながるようなコードレビューは新人になんかやらせません。
署名スパムがウザい？アカウント作って非表示に設定すればスッキリさ。
コードレビューは当然だと思っていたので (スコア:2, 興味深い)
に非常に役立ちます。
もし、コード品質があまりよくないのにコードレビューが役に立っていない
と感じるならば、外部からレビュアーをつれてくるのが大変役立つと思います。
その「外部からのレビュアー」を誰がどう選ぶのか、という問題はありますが。
が、みんなが優秀なら不要、という趣旨の論にはちょっと違和感あります。
商業的な文章は、たとえどんなプロが書いたものであってもかならず
編集者のチェックがはいりますよね。それと同じようなもので、商業的
なコードはコードのチェックをすべきで、その一手段としてコード
レビューがあると思います。なんならペアプログラミングでもよいです。
意味はあります。 (スコア:1, すばらしい洞察)
あなたが気にしてるのは、その意味を得ることが、その作業を行うためのコストに見合う価値を
持っているかって話でしょう？そんなのは状況次第で、状況設定なしに回答できるもんじゃない。
とりあえず言えることは、「コードレビューをやらなかったらどうなるのか」を試すとき、
コードレビューを日常的に受けてきてる人らのコードに対してそれをやってもあまり意味がない
ことには最低限気づくべき、ということくらいです。
程度による (スコア:1)
VB.NETで文字数をカウントする処理を書かせたら、
1.配列に1文字ずつ文字を切り出して入れて
2.配列の内容をFor文で1文字ずつ読み出してループ回数を取得
3.ループ回数を結果として返す
みたいなのを書いてくるような組織（実話）に対しては、最初にガツンと釘を刺すという意味で有効だとおもいました。
あぁ、あと、同じくVB.NETでフラグ的な位置づけの変数をStringで宣言して「"True"」とか突っ込むようなチームとか。
＃いずれも実話
Re:レベルアップが必要であることが明確であるときとレベルが不明な時には有効 (スコア:3, すばらしい洞察)
時間は掛かりますが、将来のことを考えるとレベルアップの一手段として有効だと思います
具体例がいろいろ挙がっていますが、
一般常識不足（いわゆるお約束、定石から外れている）
もっと簡単に記述できる（ライブラリにあるのに、等）
ラッパを何重にもかぶせていて実体に到達しない（不要だし、却ってわからない）
無駄に計算資源、メモリ資源を使いすぎ（それでもできるけど、意味ないし）
アルゴリズム的にダメ（経験・知識不足か、考えるのを放棄しているのか）
コンパイラで最適化が掛からない（神様がよろしくやってくれる訳ではない）
CPUの特性上遅い（キャッシュだとかレジスタの本数だとかを考えると無理がある）
読みにくい（美的センス？頭の構造がそうなのかも）
とかには効くのではないでしょうか？
Re:程度による (スコア:2, 参考になる)
>「"True"」
Cのソースでそゆの見た事ある。「condition == "true"」みたいに文字列へのポインタで比較してるの。
これ、ネットワーク界隈では結構名が売れてるブツな。
# 同じ文字列はリンカなりコンパイラなりがまとめてくれるけどさー。
Re:程度による (スコア:2)
自分も似たような状況でコードレビューしてて、とあるショートカットキーを押すと
例外吐いて落ちるイースターエッグが仕込まれているのを見つけたことがありますね。
Re: (スコア:0)
説明が足らないんじゃない？
文字のエンコード,OS,記述方向(左から右)に依存せず、制御コードを考慮した文字数カウント処理なら、１週間あっても足りないよ。
Re:程度による (スコア:1)
そこはライブラリを使うとこじゃね?
UNIX系ならwide characterとmultibyte characterの変換を知ってればいいだけだし、1分でかける。
むしろ0からそのコードを書こうとしたら止めるべきでしょ。
Windowsはしらないや。
Re: (スコア:0)
十分に優秀なプログラマーが１週間以上(かどうか知らんが)かけてそゆのをちゃんと考慮して実装してくれてんだし。
Re: (スコア:0)
で、問題あったの！？
基本的なスペック（要求時間や実行サイズ・動作確認）を満たしていれば、問題無いと思うけど。
要件として、上記のような手法はとらないと明記されていなければ、受け入れる側に
問題あり。＜イチャモンや受け入れ側のスキル不足
上記の例では、もっとスマートに書けるのはわかるし、そうするべきであるのも分かるが、
やってはイケナイ手法とまでは思えない。（環境によってはそうするのがベターな場合もある＜ループ）
コードレビューの意義って、本当は作成者の思い込みによるバグ混入（たまたま動いてた）って
ケースを見つけるためにあると思う。
＜見る（指摘する）側が余程スキルが高くないと、ほとんどの場合見つからないのが問題。
＜大概重箱の隅をつついて終了。（変数名がおかしいとか、マクロの宣言名が変とか）
Re: (スコア:0)
今ひとつわかりにくいので質問です。
VB.NETだったら文字列の長さは自分で処理書かなくても取得できますよね？
それも知らずに自分で処理を書いているような馬鹿ども、って意味でしょうか？
Re:程度による (スコア:1, すばらしい洞察)
簡単な話、「以前から使っているVBと名が付いた方が安心だし、それで困っていないから、問題とも思わない」。
細かく言うなら
-大量に居るVB6軍団がVBとVB.NETはほぼ同じだといまだに思っている人が多い。
（実際、使い始めて「えらく違う」と驚く人をここ2年くらいで目撃してます）
-その上司も同様。Cと名が付く言語よりVBと付く方が簡単だと思っている。
-まぁ実際問題、VB6以前とほぼ同じに扱ってもそれっぽくできなくもない。
（その差を埋めるくらいの応用力はあるらしい）
-C#って何？C言語は分からないよ
（実在する）
-そもそもC#とVB.NETがどういう位置づけか知らないし、興味もない。
-マイクロソフトが作る新しい言語なんてロクなもんじゃないと思ってる。
（彼らにとって2000年なんてのは「新しい」部類）
そんだけだと思う。ここに集う皆さんが常識ではじくものだけを使って食ってる人は意外にまだたくさん居るってこと。
それが原因で破綻しかければ気づくのかも知れないけれど、なまじっか動く物が出来ちゃうから変わらない。
バグが多いプログラマのコードは、見たほうがいい (スコア:1)
拡張子をチェックするのに、
if(strExt == "abc" || strExt == "ABC")
という風に書いてあるコードを見た。
"Abc" は来ないと思ってるのか～?!
new や alloc の後に、メモリが確保できたかどうかチェックしてなかったり、
try catch に挟まってたらいいけど、そうではなく..
開発PCからメモリ抜くぞ!
Re:バグが多いプログラマのコードは、見たほうがいい (スコア:4, おもしろおかしい)
if(strExt == "abc" || strExt == "ABC")
strExt が char* 型だったので、そもそも文字列比較になっていなかったというオチを期待。
うじゃうじゃ
Re:バグが多いプログラマのコードは、見たほうがいい (スコア:1)
あ!
そんなこともありました。
Re:バグが多いプログラマのコードは、見たほうがいい (スコア:1)
Javaだったら同一性を利用していたのかもしれませんよ。
パフォーマンスにシビアな携帯Javaなどではなくもない話。
とはいえ、コメントを付けておかないと、
「Stringの比較に==を使ってはいけない」
と理由も知らずに丸暗記している自称ベテランが書き直して元の木阿弥でしょうが。
Re: (スコア:0)
C++ では、new は失敗した時には、値を返さず bad_alloc の例外を送出する。 try catch で対処するしかない。
UNIX なら、limit, ulimit コマンドで datasize を変えられるから、そんな荒技必要ない。
Re:バグが多いプログラマのコードは、見たほうがいい (スコア:1)
WindowsでもXP系列なら/burnmemoryか、/maxmem [microsoft.com]を、Vista以降ならremovememoryかtruncatememory [microsoft.com]をご利用ください。
# 親コメントは外して自分のマシンにつけちゃうぞって事では？と思った上に、オフトピだけどID。
プログラマによりソースコードの品質がばらつくなら (スコア:1)
プログラマのによりソースコードの品質がばらつくなら、
コードレビューはいいかもしれません。
それより、ペアプログラミングの方がその目的を達成しやすい様な気がします。
レビューまで待たずにその場で修正させた方が良いですからね。
しかも、その場で問題点と解決方法が出てくるのでより効果的だと思います。
Re:プログラマによりソースコードの品質がばらつくなら (スコア:2, 興味深い)
コードに与える影響とチームに与える影響があると思います。
私が駆け出しの頃は、コードレビューで大変勉強させてもらいました。
チームのメンバーがみんな十分なスキルを持っているのであれば
コードレビューの効果はあまりないかもしれませんね。
(まあどんな開発手法でも達人手段の前では意味無いか)
Re:プログラマによりソースコードの品質がばらつくなら (スコア:1)
レビューの方法も大事そうですね。
指摘しっぱなしって事もありそうな気がしたもので・・・
Re: (スコア:0)
問題点の洗い出し (スコア:1)
書き方のルールがかっちり決まってて、それに合わせなくちゃというモラルもあるなら、ミスが発生しやすいポイントなどのノウハウの蓄積にはなるのではないかと。
◆IZUMI162i6 [mailto]
最近オプソの開発環境でもコードレビューツール増えてきたね (スコア:0)
次は形式手法を流行らせよう。
Re:最近オプソの開発環境でもコードレビューツール増えてきたね(荒し:-100) (スコア:1, すばらしい洞察)
＞オプソとかを流行らせないでくれ。
＞OSSって書けばもっと短いし
短くなってないやん。
しょうがない。 (スコア:0)
再利用性のあるコードを書く習慣 (スコア:0)
「〜さんのコードは見やすい」
「〜さんのコードは訳が分からない」
ということがあったりしますが、
コードの見やすさをレビューにより修正することは可能だと思います。
レビューで見てもらう訳で、何をしているか分かる/説明できるコードを
書くようになるでしょうね。ちゃんと成長する人ならば。
もちろん、ロジック的に怪しいコードを矯正する目的もありますが。
# もっとするべきだとは思いつつ、時間が・・・
syntaxとsemantics、その中間 (スコア:0)
コードに限らず、syntaxの段階で不安の散在する成果物には、手間暇かけてレビューをしないといけません。
semanticsが理解できている・定義できている優秀な集団の場合は、レビューにさほど手間はかからないでしょう。
レビューはこの辺の立ち位置によって、価値やコストが激しく変動します。
相互監視によってはたらく抑止力は、なかなかバカにできません。
形骸化しない程度に締めつけつつ、みんなが気持ち悪くなる半歩手前くらいに加減して、習慣化するのが良いようです。
この辺を上手に運用するのは、綺麗なコードを書くのと同じくらい、センスやスキルが要りますね。