クソコードレビュー会なるものに参加してきた - そんな今日この頃でして、、、

f:id:blue1st:20160818210740j:plain

転職活動でもお世話になったCodeIQで↓のような問題があったので回答を出したところ、レビュー会への招待が来ていたので行ってみた！

問題

出題はじゃんけんを行うプログラムをリファクタリングするという問題。

「標準入力として与えられた文字列を2人分のじゃんけんの手として解釈して勝敗を標準出力する」というもの。

元はnode.jsだったのだが、僕はjsが不慣れだったりもするのでperlで再現。

sub func02 {
    my ( $a, $b ) = @_;

    if ( $a eq 'g' ) {
        if ( $b eq 'c' ) {
            print "win";
        } elsif ( $b eq 'p' ) {
            print "lose";
        } else {
            print "draw";
        }
    }

    if ( $a eq 'c' ) {
        if ( $b eq 'p' ) {
            print "win";
        } elsif ( $b eq 'g' ) {
            print "lose";
        } else {
            print "draw";
        }
    }

    if ( $a eq 'p' ) {
        if ( $b eq 'g' ) {
            print "win";
        } elsif ( $b eq 'c' ) {
            print "lose";
        } else {
            print "draw";
        }
    }
}

sub func01 {
    my $line = shift;
    my ( $a, $b ) = split / /, $line;
    &func02( $a, $b );
}

my $line = <>;
chomp $line;
&func01($line);

すごーくベタな実装。

コード自体は目的どおり正しく動作するが・・・

様々な回答、「良いコード」とは

会では良いものから悪いものまで、様々な解答例を表示しながらの論評が行われた。

明らかにネタ的なものを除いても、良い例悪い例でも様々なバリエーションがあり、コードからそこはかとなく書き手のSI会社の系列や普段使っている言語が類推できるものがあったりで面白かった。

さて、本題のリファクタリングについて。

悪い例として挙げられていたものとしては、例えば教科書通りクラス化した結果かえってコードが長大になってしまっているものや、確かに答えは合っているし短くなったものの一見してもどういったニュアンスのアルゴリズムなのか読み取れないものなどもあった。

（何が何でもファクトリーモデルを適用しようとする昔の同僚を思い出してほっこりした）

十分にマシンパワーのある昨今においては、いかに「読む」ことの負荷を下げるか、可読性の高いコードこそが「良いコード」なのだ。

「良いコード」にはいくらか明白な要素はあれど画一的な基準だけでそれが定まるわけではなく、コードの目的などで評価が変わる場合もある。

今回の例題であれば問題はないが、例えば勝敗をベタに連想配列で記述するのが現実的でない分量になってしまう場合もあるし、この程度のコード量でわざわざ定数化するのはむしろ可読性が悪くなったりもする。

リファクタリングのポイント

会の中ではポイントとしては

読みやすく、理解しやすいこと
テストしやすい形であること
適切にエラーハンドリングされていること

などが挙げられていた。

1. 読みやすく

読みやすくということでいえば、例えば関数名や変数名を一見してい意図が伝わるようにすることや、適切にコメントを記述することなどが挙げられる。

sub janken {
    my ($player, $computer) = @_;

ちなみに僕は変数名や関数名あたりはちゃんと満たしたものの、

手はリストとして列挙
そのリストをハッシュ化
勝敗判定

という風な処理にしたが、これはじゃんけんという単純な目的に対しては過剰という見方が多かった。

(「じゃんけん」の要件が変わることを想定する必要性ってすごく低いわけで、パターンを最初からハッシュで列挙しちゃった方が見通しが良い等)

# 先にあいこ判定
return "draw" if $player eq $computer;

# 前の手に勝つ順番でリストを記述
my @hands = qw/g p c/;

# 手をキーとしたとき、負ける手が取れるようにする
my $lose_hand = {};
for( my $i = 0; $i < @hands; $i++ ) {
　　$lose_hand->{$hands[$i]} = $hands[$i - 1];
}

# 判定
if ( $lose_hand->{$player} eq $computer ) {
    return "win";
} else {
    return "lose";
}

※ ちなみにJSではindexOfを使って自分の手のindexを取得してそのindex+1の手=自分が勝つパターンとして判定するみたいな感じで書いた・・・が、伝わりにくかったようだ

2. テストしやすく

今回の例でいうと、実際にじゃんけんの処理を行っているfunc02にあたる部分で標準出力してしまうとテストしにくく、例えば再利用してウェブ上で表示させるようなときにそのまま使えない。

よって、じゃんけんの処理にあたる部分ではあくまで勝敗判定のみとし、標準出力はその外側で行うべき、みたいな感じ。

3. エラーハンドリング

これは僕も失念してたんだけど、例えば異常な入力があった場合への対応や、処理の外側での例外判定を行っておくと、堅牢なコードになるというような話があった。

その他には、例えば手やメッセージなんかを定数化するみたいなコードもよく見かけたけど、個人的には「読みやすさ」の定義の中に「grepで使用箇所を見つけやすいこと」も含まれてるので、よっぽど元の値が変更される機会が多いとかタイプ数が削減できるとかでない限りはむしろ可読性が下がってマイナスなんじゃないかなと思った。

まあこの辺は使用してる開発環境や言語や、組織文化によって事情が変わってくるとは思うけど。

普段全然Javascriptは書かないので、行ってみたら「お前のそれがクソコードや！」みたいな騙し討ち的なイベントだったらどうしようと不安だったが、そういうのでなくて良かったｗ

トレタさんではこんな感じのコードレビューを採用試験にも取り入れているということだが、なるほど確かにお決まりのFizzBuzz書かせるよりもより実際の能力を測れそうな気がする。

なかなか得るものの多いイベントで行ってみて良かった。