@Soseki_A (冬耳)posted at 2025-08-25

じゃんけんゲームのレビューのお願い

Q&A

Closed

Java 初心者コードレビュー

レビューのお願い

JavaでじゃんけんのCLIアプリケーションを作成しました。気づいた点があれば全て指摘いただけるとありがたいですが、特に以下の点においてレビューをいただきたいです。

保守性・可読性(命名規則は一般的であるか。命名は分かりやすいものであるか。また、もっと分かりやすい例はあるか)
拡張性(デスクトップやウェブで動くアプリケーションにしたり機能を追加したりする際、どれだけスムーズか。またはスムーズでないか)
機能面(UIは十分であるか。また、不十分なものは何か)

アプリ仕様

ファイル数：2
インターフェース：CLI
参考元:【if編】Java言語で『じゃんけんゲーム』を作る

意識した点

オブジェクト指向に則ったクラスとメソッドの定義

コード

Janken.java

package Janken;
import java.util.Scanner;
public class Janken {
    public static void main(String[] args) {
        Scanner sc = new Scanner(System.in);
        String[] hands = {"グー","チョキ","パー"};
        while (true) {
            System.out.println("じゃんけん…");
            System.out.println("グー:0, チョキ:1, パー:2");
            int playersHands = sc.nextInt();
            int computersHands = (int)(Math.random()*3);
            Judge judge = new Judge(playersHands, computersHands);
            String result = judge.result();
        if (playersHands < 0 || playersHands > 2) {
            System.out.println("不正な入力値です");
            continue;
        }
            System.out.println("プレイヤー:" + hands[playersHands]);
            System.out.println("コンピューター:" + hands[computersHands]);
            System.out.println("結果:" + result);
        }
    }
}

Judge.java

package Janken;
public class Judge {
    private int playersHands;
    private int computersHands;
    public Judge(int playersHands, int computersHands) {
        this.playersHands = playersHands;
        this.computersHands = computersHands;
    }
    public String result() {
        if (playersHands == computersHands) {
            return "あいこ";
        }
        switch (playersHands) {
            case 0:
                return (computersHands == 1) ? "勝ち" : "負け";
            case 1:
                return (computersHands == 2) ? "勝ち" : "負け";
            case 2:
                return (computersHands == 0) ? "勝ち" : "負け";
            default:
                return "不正な入力";
        }
    }
}

0 likes

4Answer

@uasi posted at 2025-08-25

保守性・可読性について

パッケージ名 package Judge はすべて小文字にするのが通例です。

int playersHands は1つの手を表しているため単数形にすべきです。また所有格の 's も省略して playerHand などとするのが一般的です。

Janken.java の if 文のインデントがずれています。

ユーザーが入力した hand 値の正しさを検証する責務がどこにあるか考えたうえで、 if 文の位置・内容を以下のように変えるべきです：

hand 値が0〜2の範囲に収まることを new Judge() の呼び出し側が保証すべきという設計にするなら、 if 文を new Judge() の前に移動する
範囲に収まっているかどうかまで Judge クラスが判定する設計にするなら、 if 文は今の位置で result の値をチェックするように変える

拡張性について

この内容や規模感だと拡張性を持たせても無駄に読みづらくなるだけなので、考えない方がいいと思います。

ひとつ言うとすれば、 judge.result() の返り値は enum にしたほうがいいでしょう。勝ち負けの表現の変更や多言語化がしやすくなります。

機能面について

sc.nextInt() は入力が整数値としてパースできなければ例外をスローします。これを処理すべきです。

シンプルなじゃんけんゲームとしては機能面の不足はないと思います。

3Like

Comments

@Soseki_A

Questioner

回答いただきありがとうございます。
私はまだプログラミングに関する知識に乏しいため、変数名における所有格の省略など、細かい点までご指摘いただき大変助かります。

ご指摘いただいた点を踏まえ、コードを修正してみました。修正前より洗練され、考慮漏れが減り、可読性も上がったと思います。また気になる点等あればコメントいただけると幸いです。
自身が思う範囲ではenumを書く場所が妥当か判断つかず、不安です。

修正コード

Janken.java

package janken;
import java.util.InputMismatchException;
import java.util.Scanner;
public class Janken {
    public static void main(String[] args) {
        Scanner sc = new Scanner(System.in);
        String[] hands = {"グー","チョキ","パー"};
        int playerHand;
        while (true) {
            System.out.println("じゃんけん…");
            System.out.println("グー:0, チョキ:1, パー:2");
            try {
            	playerHand = sc.nextInt();
            } catch (InputMismatchException e) {
            	System.out.println("整数値で入力してください");
            	sc.nextLine();
            	continue;
            }
            int computerHand = (int)(Math.random()*3);
            Judge judge = new Judge(playerHand, computerHand);
            Result result = judge.result();
	        if (result == Result.INVALID) {
	            System.out.println(Result.INVALID);
	            continue;
	        }
            System.out.println("プレイヤー:" + hands[playerHand]);
            System.out.println("コンピューター:" + hands[computerHand]);
            System.out.println("結果:" + result);
    	    //if (result != Result.DRAW) break; じゃんけんを1回のみにする。 
        }
    }
}

Judge.java

package janken;
enum Result {
    WIN("勝ち"),
    LOSE("負け"),
    DRAW("あいこ"),
    INVALID("不正な入力");
    private final String label;
    Result(String label) {
        this.label = label;
    }
    @Override
    public String toString() {
        return label;
    }
}
public class Judge {
    private int playerHand;
    private int computerHand;
    public Judge(int playerHand, int computerHand) {
        this.playerHand = playerHand;
        this.computerHand = computerHand;
    }
    public Result result() {
        if (playerHand == computerHand) {
            return Result.DRAW;
        }
        switch (playerHand) {
            case 0:
                return (computerHand == 1) ? Result.WIN : Result.LOSE;
            case 1:
                return (computerHand == 2) ? Result.WIN : Result.LOSE;
            case 2:
                return (computerHand == 0) ? Result.WIN : Result.LOSE;
            default:
                return Result.INVALID;
        }
    }
}

@uasi
enum の扱いはそれでよさそうです。

ここからは複数種類のゲームを遊べるようにしたり、戦績を保存できるようにしたりすると、抽象化や拡張性を考える課題としてちょうどよさそうに思います。
@Soseki_A
Questioner
コメントありがとうございます。
学習目的である点をご配慮いただき、課題の例まで挙げてくださり、大変感謝しております。
課題に取り組んだ後、またqiitaにてコードレビュー依頼として投稿する予定ですので、もしお目に留まりましたらまたご覧いただけると幸いです。

@utsumi2024 posted at 2025-08-25

こういう質問をする以上褒めてほしいのだとは分かったうえで改善案も出しておきます。
ちなみに、ツールでしたらそこまで設計上の問題を考える必要はないと思いますし、業務でも早く開発する方が重要なので、時間をかけてリファクタリングするというのはあんまり効果的ではないと思います。

過剰な指摘なのでリファクタリングを推奨しません。

命名について。

Judgeは動詞、resultは名詞の意味合いが強いかと思うので、
クラス名は名詞、メソッドは動詞にした方が自然だとは思う
result.judge()

名詞、動詞を分ける意識を持つとプログラムの作成はかなりすっきりする。

ちなみにじゃんけんは必ずしも1vs1で行うものではないので、プレイヤーと結果を取得できるロジックにした方が拡張性は高い。

文字列について。

ハードコーディングされているため、英語対応などがあった際にロジックを見なければならない。文字列リソースを分けて管理することを考えられる。

エントリポイントについて。

ゲーム開発での基本というのはあまり知りませんが、エントリーポイントはエントリーポイントとしての役割を持たせて、コンテンツはコンテンツとして呼び出さないと、その他のプラットフォームで共通のロジックとして起動できない。

また、CLIGame, DesktopGame, WebGameのようにクラス分離して呼び出すロジックを分ける必要がある。メインループもCLIGameに移動するべきかと思う。

合わせてこれらはインタフェースとして、ファクトリーなどで抽出できる必要がある。

副作用について。

上記エントリポイントについてと多少被るが、System.out.printlnに依存しているので、中間クラスがUIに対して結果を通知できる構造にしないと共通化することができない。

構造化されていないので、現状のソースコードの問題ではないが基本的に副作用がステートフルで存在すると処理が追いづらくなるので、依存性注入などでステートレスなクラス、メソッドにした方がよい。

不正入力について。

バリデーションがあるなら即行う。バリデーション後はバリデーション済みであるというクラスにした方がよい。バリデーション済みであるというインスタンスにすることで、この入力が必要なクラスで再検証する必要がない。

public Judge(Hand playersHands, Hand computersHands)
よりも
public Judge(ValidHand playersHands, ValidHand computersHands)
の方がミスがないという話。

エラー機構について。

不測のエラーを捕捉する機構がないので、例外が起きるとクラッシュする。

1Like

Comments

@Soseki_A
Questioner
回答ありがとうございます。
褒めていただけるのも大変ありがたいですが、むしろ改善案を求めた投稿だったため、大変助かります。何ができていないかが分からないレベルなため、気づいた点があれば全てご指摘いただけると幸いです。

ちなみに、ツールでしたらそこまで設計上の問題を考える必要はないと思いますし、業務でも早く開発する方が重要なので、時間をかけてリファクタリングするというのはあんまり効果的ではないと思います。

確かに業務でも時間をかけたリファクタリングは求められないように思いますが、読みやすく分かりやすいコード書けるようになりたいと思っているので今回このようなレビューを依頼した次第です。
今回のような学習のためではない、業務上などの開発では、おっしゃる通り命名の修正などに時間をかけないよう心がけます。
以下、理解した点、理解しきれていない点をまとめました。

理解した(と思う)点

命名について

確かに動詞と名詞の意味合いを意識できていませんでした。挙げていただいた例の通り命名する方が自然に感じられます。また、じゃんけんが1vs1に限らないという考慮も確かに抜けていました。

文字列、エラー機構について

他の回答者様にもご指摘いただき、enum，try-catchを使用するよう変更しました。

理解しきれていない点

私の知識不足により理解しきれていない点です。
ファクトリーや依存性注入、エントリポイントといった言葉すら知識としてなかったため、勉強の必要性を感じました。

エントリポイント、副作用、不正入力について

共通化したいならmainメソッド内では各UIのクラスのインスタンスを呼び出すだけにする、という認識でよいか？
また、依存性注入とは共通のロジックを書いたクラス、メソッドの出力先だけ変えればよいようにするという認識でよいか？
System.out.printlnに依存するとロジックがUIに依存することになり、共通化できないのは理解しました。
また、UIごとにクラスを作った時、バリデーション済みのクラスを作った方が、クラスそれぞれにバリデーションを書かなくてよいということも理解しました。
@utsumi2024

共通化したいならmainメソッド内では各UIのクラスのインスタンスを呼び出すだけにする、という認識でよいか？

とても合理的にいうなら、単一のクラスにはそれ以外の役割を持たせたくないという理論です。
mainメソッドにはそれがプログラムのエントリー(main)以外の処理を持たせたくなく、必要なアプリケーションなりを呼び出すようなロジックにしたいのです。

また、依存性注入とは共通のロジックを書いたクラス、メソッドの出力先だけ変えればよいようにするという認識でよいか？

とても合理的にいうなら、関心の分離を行えるところは行おうという話です。
同じ認識かわかりませんが、依存性注入は依存性逆転のパターンで、そのクラスとしての責務が追えないために委譲したい処理を外部から受け取ることで、依存の差し替えが容易になるということです。コード的にもどこに副作用があるのか分かりやすくなるケースもあります。

ちなみに。。。
ご存じかもしれませんが、Int以外が入力に来るというのは事前にわかっていることなので、それをエラーとして捕捉するのは一般的には推奨されません。(エラーによるプログラミングと呼ばれます。)

Int以外が来ると事前にわかっているのであれば、エラーが出る前に処理できるはずですし、エラーはコストが高いです。

蛇足
もしあなたが設計に関して理解できていれば、
じゃんけんで「どれを出すかという処理」自体が同じような役割を持ったものであることに気づくと思います。
ランダムに出すか、CLIから入力されるか、DBから読み取るか、API(ネット通信など)から読み取るか、のように、実際のロジックを書かなくてもクラスの設計ができることに気が付くでしょう。
ランダムに出すというのは別にCPの専売特許でもないですから、ユーザーが自身で出す以外に「ランダムに出す」という選択をできるUIも考えると共通化とクラス分離に関しては少し面白いかもしれません。
(もちろん。どのように実装するかは人それぞれですが。)

@shiracamus posted at 2025-08-26

オブジェクト指向に則ったクラスとメソッドの定義

オブジェクト指向設計（クラス構成）についてですが・・・

人間(Human) とコンピュータ(Computer) が手(Hand) を出してジャンケンするので、それらをクラスにしてみるのはいかがでしょうか？
両者ともプレイヤー(Player)で、じゃんけんの手の決め方は違いますが、相手より強いか判断し、勝ったか負けたか判定できる存在（役者）かと思います。
Jankenクラスはゲーム進行役、進行係。
クラスは役者、担当、係。

参考になりそうな資料：オブジェクト指向開発論

Janken.java

import java.util.Scanner;
import java.util.Arrays;

enum Hand {
    GU("グー"), CHOKI("チョキ"), PA("パー");

    private final String name;

    Hand(String name) {
        this.name = name;
    }

    @Override
    public String toString() {
        return name;
    }
}

abstract class Player {
    protected Hand hand = Hand.GU;  // 最初はグー

    abstract public void jankenPon();  // 手の決め方はサブクラスに任せる

    public String hand() {
        return hand.toString();
    }

    private boolean isStrongerThan(Player other) {  // otherより強ければtrue
        return this.hand == Hand.GU && other.hand == Hand.CHOKI ||
               this.hand == Hand.CHOKI && other.hand == Hand.PA ||
               this.hand == Hand.PA && other.hand == Hand.GU;
    }

    public String judge(Player other) {
        return this.isStrongerThan(other) ? "勝ち" :
               other.isStrongerThan(this) ? "負け" :
                                            "あいこ";
    }
}

class Human extends Player {
    private static final String SELECTION =
        String.join(", ", Arrays.stream(Hand.values())
                                .map(hand -> hand.ordinal() + ":" + hand)
                                .toArray(String[]::new));

    @Override
    public void jankenPon() {
        Scanner sc = new Scanner(System.in);
        while (true) {
            System.out.println("じゃんけん…");
            System.out.println(SELECTION);
            try {
                hand = Hand.values()[sc.nextInt()];
                return;
            } catch (Exception e) {  // 数字以外、範囲外数字
                System.out.println("不正な入力値です");
                sc.nextLine();  // 不正な入力値を読み捨てる
            }
        }
    }
}

class Computer extends Player {

    @Override
    public void jankenPon() {
        Hand[] hands = Hand.values();
        hand = hands[(int)(Math.random() * hands.length)];
    }
}

public class Janken {

    public static void main(String[] args) {
        // みんな集まれ～、最初はグー
        Player human = new Human();
        Player computer = new Computer();

        // じゃんけんポン！
        human.jankenPon();
        computer.jankenPon();

        System.out.println("プレイヤー:" + human.hand());
        System.out.println("コンピューター:" + computer.hand());
        System.out.println("結果: " + human.judge(computer));
    }
}

UMLクラス図：

1Like

Comments

@Soseki_A
Questioner
回答ありがとうございます。
オブジェクト指向設計はできているか？またできていなければどう実現するか？といった視点での回答はまだいただけていなかったので大変うれしいです。
コンピュータもプレイヤーと考え抽象クラスを作る点など確かにオブジェクト指向的だと思いました。
また、例としてコードを全て貼ってくださったので、クラスを役者や係として考えて作るとはどういうことか？というのがよく理解できました。
信頼度が高そうな資料の提示もありがたいです。

@shiracamus

抽象クラスPlayerをHumanとComputerが継承して手の選び方を具象実装するコード（デザインパターンの「Template Methodパターン」）を示しましたが、手の選び方をPlayerの頭脳・戦略として持たせて委譲する「Stategyパターン」のコードも示しておきます。
世間では、継承するより委譲する方がいいと言われています。

import java.util.Scanner;
import java.util.Arrays;

enum Hand {
    GU("グー"), CHOKI("チョキ"), PA("パー");

    private final String name;

    Hand(String name) {
        this.name = name;
    }

    @Override
    public String toString() {
        return name;
    }
}

interface JankenStrategy {  // ジャンケンの手を選ぶ戦略
    public Hand jankenPon();
}

class Player {
    private Hand hand = Hand.GU;  // 最初はグー
    private final JankenStrategy strategy;

    public Player(JankenStrategy strategy) {
        this.strategy = strategy;
    }
    
    public void jankenPon() {
        hand = strategy.jankenPon();
    }

    public String hand() {
        return hand.toString();
    }

    private boolean isStrongerThan(Player other) {  // otherより強ければtrue
        return this.hand == Hand.GU && other.hand == Hand.CHOKI ||
               this.hand == Hand.CHOKI && other.hand == Hand.PA ||
               this.hand == Hand.PA && other.hand == Hand.GU;
    }

    public String judge(Player other) {
        return this.isStrongerThan(other) ? "勝ち" :
               other.isStrongerThan(this) ? "負け" :
                                            "あいこ";
    }
}

class ManualStrategy implements JankenStrategy {
    private static final String SELECTION =
        String.join(", ", Arrays.stream(Hand.values())
                                .map(hand -> hand.ordinal() + ":" + hand)
                                .toArray(String[]::new));

    @Override
    public Hand jankenPon() {
        Scanner sc = new Scanner(System.in);
        while (true) {
            System.out.println("じゃんけん…");
            System.out.println(SELECTION);
            try {
                return Hand.values()[sc.nextInt()];
            } catch (Exception e) {  // 数字以外、範囲外数字
                System.out.println("不正な入力値です");
                sc.nextLine();  // 不正な入力値を読み捨てる
            }
        }
    }
}

class RandomStrategy implements JankenStrategy {

    @Override
    public Hand jankenPon() {
        Hand[] hands = Hand.values();
        return hands[(int)(Math.random() * hands.length)];
    }
}

public class Janken {

    public static void main(String[] args) {
        // みんな集まれ～、最初はグー
        Player human = new Player(new ManualStrategy());
        Player computer = new Player(new RandomStrategy());

        // じゃんけんポン！
        human.jankenPon();
        computer.jankenPon();

        System.out.println("プレイヤー:" + human.hand());
        System.out.println("コンピューター:" + computer.hand());
        System.out.println("結果: " + human.judge(computer));
    }
}

UMLクラス図：

@yukiyama2003 posted at 2025-09-04

保守性・可読性、拡張性、機能面とのことですが、ここまで短いプログラムだと
見通しの良いほうがいいのではないかと思いました。

まず気になったのはmainメソッドの中にいろいろ書きすぎていること。
自分の手の入力チェックまで書いているので、メインの流れがどこなの？と思いました。
自分の手の入力チェックまで、メインの流れでやるべきかなぁ？と
もしやるなら自分の手を入力した直後に有効な値が判定してやり直すという風に直すとメソッド単位で機能が完結して
追いやすいかなと（GetPlayersHandsみたいに）

同じようにコンピュータの手をランダムで決めてますが、ここもGetComputersHandsメソッドに持って行って
その中だけで完結する方がいいと思います。
どんな処理をしているかはわからないけれどコンピュータがどんな出した手を出したか？さえわかれば
メインの流れを読むときに追いやすいです。

あと、勝負のジャッジ、練習のために別クラスに分けてあるのだと思いますが
ここまで短い処理だと１クラス内にまとめたほうが見通しが良いと思い、１クラスにまとめました。
こうすることで別のクラスを newして結果を得るという冗長な部分をなくしました。
また、ジャッジのswitchのdefault、こちらは自分の手で想定外の値が入ってこなければdefaultが要らなくなるはずです。
どんな値が入ってくるかわからない場合の安全装置として入れておくのはありですが今回の場合不要と思います。

メインの流れをなるべくシンプルにして、機能ごとにメソッドをわけてその中で機能が完結すると良いかなと思います。

package Janken;
import java.util.Scanner;
public class Janken {
    String[] hands = {"グー","チョキ","パー"};

    public static void main(String[] args) {
        while (true) {
            judge(GetPlayersHands(),GetComputersHands());
        }
    }

    public int GetPlayersHands() {
        Scanner sc = new Scanner(System.in);
        while (true) {
			System.out.println("じゃんけん…");
			System.out.println("グー:0, チョキ:1, パー:2");
			int playersHands = sc.nextInt();
			if ( 0 <= playersHands || playersHands <= 2) return playersHands ;
			System.out.println("不正な入力値です");
		}
    }

    public int GetComputersHands() {
 		return  (int)(Math.random()*3);
    }

    public Judge(int playersHands, int computersHands) {
        String result= "あいこ"; 
        if (playersHands != computersHands){
	        switch (playersHands) {
	            case 0: result = (computersHands == 1) ? "勝ち" : "負け";
	            case 1: result = (computersHands == 2) ? "勝ち" : "負け";
	            case 2: result = (computersHands == 0) ? "勝ち" : "負け";
	        }
        }
        System.out.println("プレイヤー:" + hands[playersHands]);
        System.out.println("コンピューター:" + hands[computersHands]);
        System.out.println("結果:" +  result);
    }
}

0Like

Are you sure you want to delete the question?

じゃんけんゲームのレビューのお願い

レビューのお願い

アプリ仕様

意識した点

コード

4Answer

保守性・可読性について

拡張性について

機能面について

Comments

修正コード

命名について。

文字列について。

エントリポイントについて。

副作用について。

不正入力について。

エラー機構について。

Comments

理解した(と思う)点

命名について

文字列、エラー機構について

理解しきれていない点

エントリポイント、副作用、不正入力について

Comments

Your answer might help someone💌