しょぼちむさんと並んで最近Twitter上でよく見かけるよこなさんが、『何も作ったことない若者がじゃんけん作ったはなし』というブログで、じゃんけんのプログラムをJavaで作っていました。
初めて1からJavaプログラムを書いたようで、初めてでこれだけ出来るのはすごいと思います。
というのと、初めての人が書くとこうなるんだ!(自分ではこういう発想は出来ないぞ)という意味で、すごく面白いプログラムです。
自分が書いたプログラムを公開するのもマサカリを受け止めると書くのもすごく勇気がいるので、素晴らしいの一言に尽きます。
という訳で、マサカリとかコードレビューとかではないですが、自分が気になったポイントを少し挙げてみたいと思います。
14~16行目、定数定義
Javaの場合、定数(固定値)を定義する場合は、static finalにすることが多いです。
ただ、定数→変数という順序で定義されているのは良いと思います。
21~22行目、フィールド定義
可視性(privateとか)が何も付いていないのが気になります。
クラス内で何も付いていない場合はパッケージプライベートであり、自分の場合、パッケージプライベートにするのは「本来はprivateだけれども内部クラスあるいはテストクラスからアクセスする」という場合だけです。
今回の場合だと、テストクラスからこれらにアクセスできる必要性は無さそうな気がします。
あと、resultsという変数名だと、じゃんけんした結果を保持するような感じがします。
(以降、メソッド名や変数名については言及しません。「じゃあどういう名前にすべきか」という答えに自信が無いので(爆))
BufferedReaderのインスタンス生成をフィールドの初期化で行っているのもちょっと違和感がありました。
仮にファイルから生成するような場合だと、インスタンス生成過程でFileNotFound等の例外が発生する可能性があります。つまりIOExceptionを処理する必要があるのですが、これはコンストラクターにthrowsを書くことになります。たぶん知らないとハマりますので、自分はそういう処理ではフィールドでインスタンス生成しません。
ただ、このBufferedReaderのクローズをしてないのは良いですね。
普通はBufferedReaderインスタンスを生成するとクローズしないといけないのですが、今回はSystem.inを使っているので、これをクローズするとそれ以降でコンソール入力できなくなってしまいます。
System.inという特殊ケース専用の話ですが。
31行目、decideShapeメソッド
乱数を使っているので、コンピューター側の手を返すメソッドですね。
返している値が1~3のStringなので、じゃんけんの手をそういうIDで管理しているのかと思いました。
(SIerだと、コードをStringで扱うのもよくやるので^^;)
で、じゃんけんの手のように有限の場合は、IDはenumで定義するのが常套手段だと思います。
36行目、transferNumメソッド
switchの変数にStringを使っているのが偉い(笑)
JDK1.7から入った機能なので、昔からJavaをやっている人なら逆にやらない(知らない)可能性が高いです。
(ちなみに今回は関係ないですが、switchに渡す値がnullだとNullPointerExceptionが起きるので注意が必要です)
ここで返している値はIDに相当する使い方をしているので、クラスの先頭で定義しているROCK, PAPER, SCISSORSという定数を返すようにすべきでしょう。
また、引数が仕様の範囲外の値だった場合にIllegalArgumentExceptionを投げるのは一般的ではありますが、この例外を投げなくてはいけない状況というのは、バグがある時です。(値を渡す側が悪い、という場合に使う)
今回のようにユーザーの入力値の精査(バリデーション)を行う場合は、IllegalArgumentExceptionは相応しくないと思います。
(なお、IllegalArgumentExceptionを使う場合は、例外メッセージ内に入力値を含めるべきだと思います。そうしておけば、例外メッセージを見ただけでどんな値を入れたのか分かります。
(ただし、業務アプリであれば、ユーザー名やパスワード等を出力してしまうとログに残るので、出していいかどうかはポリシー次第かなー))
で、さっきのdecideShapeは1~3の文字列を返し、transferNumでそれをさらにグー・チョキ・パーというIDに変換しているので、余計なステップが入っているように感じます。decideShapeが直接IDを返せばいいのに、と。
まぁ、decideShapeの仕様が「(人間がキー入力するのと同じ感じで)コンピューターのキー入力結果を返す」のであれば、これでもいいのかもしれません。
49行目、isTiedメソッド
変数を使った文言(メッセージ)をコンソールに出力したい場合、個人的にはprintfを使う方が好みです。
System.out.println("あなたは"+ formedShape +"、わたしも"+ selectedShape +"。あいこ!もう1回!");
↓
System.out.printf("あなたは%s、わたしも%s。あいこ!もう1回!%n", formedShape, selectedShape);
もっと言うと、人間に見せるためのメッセージはMessageFormatを使っておくと、メッセージであることが明確になります。(多言語対応もしやすいw)
(もっと言うと、Javaには文字列内に値を埋め込む構文が無いからなぁ)
58行目、printResultメソッド
throws NullPointerExceptionが書かれていますが、普通はNullPointerException(というかRuntimeExceptionのサブクラス)はthrowsには書きません。
特にNullPointerException(略称NPE)はシステムが出力するものなので、基本的には自分で扱わないようにすべきです。(NPEを投げたくても、別の例外(業務アプリの仕様で定められた例外)にすべき)
70行目、matchメソッド
Javaの場合、ローカル変数をメソッドの先頭で宣言だけすることは(普通のコーディング規約では)しません。
使う箇所で定義します。
ってゆーか、inputTextはそうなってるじゃんw
BufferedReaderのreadLineメソッドは、EOF(end of file、入力の終了)になるとnullを返します。
したがって、nullチェックが必須です。
コンソール入力の場合でもEOFは起きます。WindowsだとCtrl+ZでEOFになります。UNIXだとCtrl+DでEOFになると思います。
それから、matchメソッド内のメソッド呼び出しのレベルに違和感がありました。
引き分けかどうかの判定はしているのに、勝ち負けの判定が見当たらないんです。
引き分けでなかったらいきなり結果表示(printResult)を呼んでいるので、あれ?となりました。
最後に、matchメソッドはじゃんけんを繰り返し行う場合のループも兼ねているようですが、ループのさせ方に感激しました!
再帰呼び出しを使ってループさせる方法は、関数型プログラミングだとこういう例がよく出てきますが、自分は(知識では知っているけれども)まず思い付きませんorz
ただ、残念なことに、Javaでは末尾再帰最適化は行われないのです。つまり、延々と繰り返しじゃんけんを行っていると、どこかでスタックオーバーフローが発生してしまうでしょう。
(そうでなくとも、途中で例外が発生したら、スタックトレースが酷い事になります^^;)
105行目、mainメソッド
mainメソッドの引数が可変長引数で定義されているのが良いですね。通ですね(笑)
(JDK1.5から書けるようになった方法なのです)
例外のマルチキャッチを使っているのもいいですね。(JDK1.7から書ける方法)
ただし、メッセージだけ出して、スタックトレースを出していないのは駄目です。
一番単純にはe.printStackTrace()でスタックトレースを表示できます。
って、よく見たらNullPointerExceptionをキャッチしているんだ^^;
NPEは普通はキャッチしなくていいと思います。(個人的には、NPEが発生するのは実行を続けるのが不可能なバグだと思うので、mainの外まで伝播させてしまってよいと考えます)
また、IllegalArgumentExceptionをキャッチしていますが。
これはtransferNumで発生したものを処理する想定のようですが、キャッチする場所が良くないですね。
transferNumの呼び出し箇所でキャッチすべきです。
mainでキャッチすると、そのままプログラム終了ですよね^^; キーの再入力に戻る方がいいでしょう。
(あと、もし仮にdecideShape(コンピューターの手を返す)が1~3以外を返したらtransferNumでIllegalArgumentExceptionが発生しますが、今のメッセージだと、それも人間が入力した手のように誤解しちゃいますよね)
そういえば、mainメソッドをクラスの先頭に書くか末尾に書くかはJava界でも定まっていないような気がしますが(決まってたら教えてちょ)、それ以外のメソッドに関しては、並び順が概ね標準化されています。
aメソッドがbメソッドを呼び出しているとき、a→bの順に定義します。(aのすぐ下にbを書く)
C言語だと、先に宣言してある関数しか呼べない為、b→aの順に書くことも多いですが。(aの前にbを書く)
さて、よこなさんのプログラムに触発されて、さくらばさんもじゃんけんプログラムを書いています(笑)
自分も、ちょっと書いてみました。(後編へ続く)
※コメント投稿者のブログIDはブログ作成者のみに通知されます