実際にコードレビューしてみた
「解答コードを見ると全部で55行くらいあり関数化もされていて、シンプルに書いてあったと思います。ただコメントが記述されていませんでした」
実際の採点テンプレートとレビュー結果は次図の通り。
本セッションでは、各評価項目について実際に詳しく解説したが、本レポートでは代表的な項目に限って紹介する。まず全体的に可読性の高いコードか調べるため、メインロジックの記述内容を見た。すると、csv_info変数があるが、一度も利用されていないことが分かった。また、scores変数とplayers変数・plyayer_score変数を唐突に利用していた。
「コードをよく読むと、唐突に出てきた変数をすべて先頭行でグローバル変数として定義していました。この時点でグローバル変数に中身は入っていないので、各関数でこのグローバル変数を自由に読み書きしているようでした。おそらく最初は関数化せずに、頭から作っていって、それを処理のブロックごとに関数化したのではないかと考えました」
これは、もちろんグローバル変数の汚染につながりやすい。あるべきコードとしては、グローバル変数をすべて削除し、main()関数のなかで各関数を呼び出して、その戻り値をmain()関数のスコープ内だけで利用する形にしたい。
次は個別の関数を調べていった。ここでは、load_csv()関数の責務に注目した。すると、players変数とscores変数を返しており、それからjoin_list()関数で、この2つの値を連結したplayer_score変数を作成していた。
「このデータをよく見みると、インデックス値は同じでplayers変数はプレイヤーIDだけ、scores変数はそのプレイヤーIDの人が取った得点をリストで含んだデータでした。インデックス値が同じだけど2つの別データは、扱いづらい不安定なものだと考えています。どちらか片方だけデータを挿入すると全部ずれてしまう。そういうものがバグを生み出しやすいと思います」
load_csv()関数が辞書型でデータを返すようにすれば再度連結しなくても良くなる。次図の右下のように、プレイヤーIDに対するスコアのリストを保持するのだ。
いずれも現場でコードを書くにあたって必要となる実践的なテクニックばかりだ。本セッションのコードレビューでは、この他にも「プログラムの処理の流れが適切な粒度で分離されているか」「関数名と変数名のリファクタリング」「唐突なマジックナンバーの使用と、問題に対応した数字の使用」「ファイルI/O周りの処理」「メモリとCPUの効率」など、多くのポイントについて詳しく解説した。
ゆめみでは、このようやレビュー結果を、テキストでキレイにまとめてフィードバックするのは難しいので、評価でよかったところと改善ポイントをセットにして不合格になった応募者にも伝えているそうだ。
「もしかすると応募者によってはピンとこないところがあるかもしれませんが、『我々はこのように見ました』という内容を改善ポイントとしてお伝えするようにしています」
なお、講演後のAsk The Speakerで「合否関係なくレビューのコメントを応募者に伝えるのはなぜでしょうか？」という質問があった。
これについて仲川氏は「不合格だった場合も6カ月とか一定の期間をおいて再チャレンジできる仕組みを用意している」と説明した。コードレビューの内容をちゃんと自分で理解して、もう一度チャレンジして、そこが改善されていたらそれを評価するというのだ。
「あとは、学生さん全体への貢献といった側面もあり、こういうことをやっている会社だよと見ていただければと思います」
