私自身が表題の問題を解いた時のプログラムについて解説します。
問題の詳細は「クソコードを直してあげてください」(CodeIQ)を参照してください。
あなたは新人の教育係になりました。
新人には、次の課題を出しました。
じゃんけんの勝ち負け判定プログラムを作って欲しい
標準入力から、人間の手、コンピュータの手が、アルファベットのg,c,p (グーチョキパー)でスペース区切りで入ってくる。
人間が勝ちなら、win
人間が負けなら、lose
あいこなら、draw
と標準出力に出力して欲しい。
開発言語はNode.js。環境は、CodeIQ実行くん(https://codeiq.jp/tools/sandbox/ )上のnode.js 0.12で動くことを前提とする。
例)
STDIN
g c
STDOUT
win
そして新人君が書いたコードがこれだ!
function func02(a,b){ if (a== "g"){ if(b=="g"){ console.log("draw"); }else if(b=="c"){ console.log("win"); }else if(b=="p"){ console.log("lose"); } } if (a== "c"){ if(b=="g"){ console.log("lose"); }else if(b=="c"){ console.log("draw"); }else if(b=="p"){ console.log("win"); } } if (a== "p"){ if(b=="g"){ console.log("win"); }else if(b=="c"){ console.log("lose"); }else if(b=="p"){ console.log("draw"); } } } function func01(buff){ var ary = buff.split(" "); a = ary[0]; b = ary[1]; func02(a,b); } var readline = require('readline'); var reader = readline.createInterface({ input: process.stdin, output: process.stdout, terminal: false }); reader.on('line', function (buff) { func01(buff); });
このまま、コピー&ペーストしても、入出力はあってる。だけど、入出力のテストケースは正解したままで、このコードを保守しやすくして欲しい。本来はコミットログに書くべきだが、どのようにリファクタリングしたかは、コメントに書き残して提出して欲しい。
JavaScriptで解答しています。
/* * コミットログ的なもの * 関数名がひどいのでfunc01() -> janken()、func2() -> checkJanken()に修正。 * コード量的に1関数にまとめても良いが、checkJanken()は拡張時にこれだけを使えるかもしれないので別関数とした。 * checkJanken()は汎用性を高める為、数値を返すように修正。 * checkJanken()の判定は一目で対応がわかるように連想配列のマトリックスとした。 * janken()は入力値のパースと印字の為の関数とした。 * エラーメッセージをサービスしておいた。 */ /** * じゃんけんしてaが勝ちなら1、負けなら-1、あいこなら0を返す。 * @param a 人間の出した手 * @param b コンピュータの出した手 * @return 1:人間勝ち、0:あいこ、-1:コンピュータ勝ち */ function checkJanken(a, b){ var JANKEN = { "g": {"g": 0, "c": 1, "p": -1}, "c": {"g": -1, "c": 0, "p": 1 }, "p": {"g": 1, "c": -1, "p": 0 }, }; return JANKEN[a][b]; } /** * 入力されたスペース区切りの文字列をパースして結果をチェックし、win、lose、drawのいずれかを返す。 * @param 入力値 * @return "win", "draw", "lose"。エラー時はエラーメッセージ。 */ function janken(buff){ var RESULT = {"-1": "lose", "0": "draw", "1": "win"}; var HANDS = {"g":true, "c":true, "p":true}; var hand = buff.split(" "); if(hand.length != 2){ return "Error: Input must have 2 values."; } if(!(hand[0] in HANDS) || !(hand[1] in HANDS)){ return "Error: input must be 'g', 'c' or 'p'."; } var ret = checkJanken(hand[0], hand[1]); return RESULT[ret.toString()] } var readline = require('readline'); var reader = readline.createInterface({ input: process.stdin, output: process.stdout, terminal: false, }); reader.on('line', function (buff) { console.log(janken(buff)); });
★1つの問題です。コード自体は動くからでしょうか?
コメントに書くべきことはほとんど書いてありますが、一応説明します。
func01()、func02()はちょっとひどいので付け直しました。
関数は元の通り2つにしています。コード量的には1つにしても良いと思いますが、じゃんけんの結果判定関数(checkJanken())に汎用性を持たせるのも良いかと思い、パースと結果を返すための処理とじゃんけんの結果判定の2つの関数に分けました。
入力値をパースして結果文字列を返します。
入力チェックも追加し、仕様にはありませんでしたがエラー出力も記述してみました。入力チェックはcheckJanken()でチェックし、入力エラーは例外とした方がよかったでしょうか?
結果判定処理ですが、非常に簡単になっています。
というか、結果を連想配列のマトリックスにしてその値を返すだけです。
かつて『じゃんけん「グアバ・チョコレート・パイナップル」ゲーム』という問題をやった時に同じ手法を用いています。
ポイントはif文をなくすことです。私の考えですが、if文はコードの見通しを悪くします。if-(else if)-elseが出てくるとそこで条件ごとに考えを保留してコードを読まなければならなくなるためです。
この場合ならswitch-caseを使えばもう少しは読みやすくなります。当然、caseの中は別関数にしてcase以下にだらだらロジックを書いてはいけません。
ですが、この場合私が書いたように連想配列の連想配列を使うか、2次元配列を使って結果を返すようにするのが一番見通しが良いと思います。何と何だったら結果が何になるかが一目でわかりますから。
寝ようと思って布団に入ったままノートPCでCodeIQを見たら問題があったので、そのままチャチャっとやってしまいました。先にも書きましたがエラーチェックはcheckJanken()でやって例外を投げるようにした方がよかったような気がしています。その他、両方の関数をオブジェクトのメンバにするかクラスのメンバにしてしまう(名前空間に閉じ込める)のと、checkJanken()の引数を{human:"値", "computer":"値"}のようなオブジェクトにした方がよかったような気がしています。