質問をすることでしか得られない、回答やアドバイスがある。

15分調べてもわからないことは、質問しよう!

ただいまの
回答率

88.20%

ババ抜きで手札から同じカードを2枚捨てるメソッドが作れないです

解決済

回答 3

投稿 編集

  • 評価
  • クリップ 0
  • VIEW 4,235

itchee92

score 7

 前提・実現したいこと

Javaでトランプのババ抜きを作っていてその中でarrayクラスの配列を参照して同じカードが2枚あったら捨てる(削除する)メソッドを作りたいのですが難しいです。
今はfor文2回で繰り返しているのですがおそらく配列の要素数が変わるためエラーが起きて実行できません。

 発生している問題・エラーメッセージ

Exception in thread "main" java.util.ConcurrentModificationException

 該当のソースコード

public ArrayList<Card> checkSameCards(Player p) {
        ArrayList<Card> cards = p.getCardHand().getCards();
        ArrayList<Card> discards = p.getCardHand().getCards();
        for(Card c1: cards) {
            for(Card c2: cards) {//エラーはこの行に出てます
                if(c1.getNumber() == c2.getNumber()) {
                    if(c1 != c2) {
                        discards.add(c1);
                        discards.add(c2);
                    }
                }
            }
        }
        return discards;
    }

以下がカードが同じかをチェックして捨てるプログラムです

for(Player p: playerList) {  //実行しているgetGameメソッドの一部
            if(p instanceof User) {
                p.getCardHand().showAllCards();
              } 
            ArrayList<Card> cards = judge.checkSameCards(p);
            for(Card c: cards) {
                p.discardCard(c);
            }
        }
        for(Player p: playerList) {
            if(p instanceof User) {
              p.getCardHand().showAllCards();
            }       
        }

参考までにプレーヤクラスも載せときます

public abstract class Player {  //プレーヤクラス
    /** プレーヤー名 */
    private String name;
    /** 手札 */
    private CardHand hand = new CardHand();

    /**
     * 名前を指定して、じゃんけんのプレイヤーを作る
     * @param name プレイヤー名
     */
    public Player(String name) {
        super();
        this.name = name;
    }

    /**
     * 自分の名前を外部に取得させる
     * @return 名前
     */
    public String getName() {
        return name;
    }

    /**
     * 自分の手札を外部に取得させる
     */
    public CardHand getCardHand() {
        return hand;
    }

    /**
     * 自分の名前をセットする
     * @return セットする名前
     */
    public void setName(String name) {
        this.name = name;
    }

    /**
     * 相手の手札から手札を一枚引く
     * @return 引いたカード
     */
    public void getCardHand(Player p) {
        Card c = p.hand.getCardAtRandom();
        hand.addCard(c);
    }

    public void addCardHand(Card c) {
        hand.addCard(c);
    }

    public void discardCard(Card c) {
        hand.getCards().remove(c);
    }

    /**
     * 手札の枚数を表示する
     */
    public void showNumberOfCards() {
        System.out.println(name+"さんの手札は"+hand.getNumberOfCards()+"枚です.");
    }

}
  • 気になる質問をクリップする

    クリップした質問は、後からいつでもマイページで確認できます。

    またクリップした質問に回答があった際、通知やメールを受け取ることができます。

    クリップを取り消します

  • 良い質問の評価を上げる

    以下のような質問は評価を上げましょう

    • 質問内容が明確
    • 自分も答えを知りたい
    • 質問者以外のユーザにも役立つ

    評価が高い質問は、TOPページの「注目」タブのフィードに表示されやすくなります。

    質問の評価を上げたことを取り消します

  • 評価を下げられる数の上限に達しました

    評価を下げることができません

    • 1日5回まで評価を下げられます
    • 1日に1ユーザに対して2回まで評価を下げられます

    質問の評価を下げる

    teratailでは下記のような質問を「具体的に困っていることがない質問」、「サイトポリシーに違反する質問」と定義し、推奨していません。

    • プログラミングに関係のない質問
    • やってほしいことだけを記載した丸投げの質問
    • 問題・課題が含まれていない質問
    • 意図的に内容が抹消された質問
    • 過去に投稿した質問と同じ内容の質問
    • 広告と受け取られるような投稿

    評価が下がると、TOPページの「アクティブ」「注目」タブのフィードに表示されにくくなります。

    質問の評価を下げたことを取り消します

    この機能は開放されていません

    評価を下げる条件を満たしてません

    評価を下げる理由を選択してください

    詳細な説明はこちら

    上記に当てはまらず、質問内容が明確になっていない質問には「情報の追加・修正依頼」機能からコメントをしてください。

    質問の評価を下げる機能の利用条件

    この機能を利用するためには、以下の事項を行う必要があります。

回答 3

checkベストアンサー

+6

Playerの実装が不明なのですがエラーの内容から原因の推測はできそうです。

多分cards, discardsは「同一のリスト」を指しています。cardsとdiscardsが同一だと困ったことが起きます。

それを知るにはまず、リスト1のコードが間違いと知ることが第一歩になります。

リスト1

for (Card c1: cards) {
  cards.add(...);
}

このfor文(詳しく言えば自動的に生成されるIterator<Card>)は、正しい動作を保証しつつ効率よく動作させる都合上「列挙している最中にcardsの要素の追加は禁止」ということになっています。

「今列挙している要素より前の方に追加するのはできてもいいんじゃないの?」と疑問に思うかも知れません。論理的には確かにそういう実装も可能ではありますがそれはIteratorの実装を予想外に複雑にするためJava標準ライブラリーの設計者はもっと単純な戦略(列挙している間はコレクションへ追加しちゃダメ)を採用したのだと思います。

さて、ご質問のコードでcardsとdiscardsがもし同一ならdiscardsへaddするということは即ちcardsへaddすることと同じことを意味します。それはリスト1がNGであるのと同じ理由でNGなのです。


次に対処ですが・・・それを述べる前にそもそもdiscardsの初期化が正しいかを考えてみてほしいです。discardsはループの開始時点では「空」であってほしいはずです。一致したカードを見つける度にdiscardsへ追加していくのが目的なのですから。それを正すことができれば、自然に先に述べた問題も解消し、万事まるく収まると思います。


ところで質問者さんがもともと質問本文に張り付けたのは以下のような雰囲気のコードです。(若干インデントが乱れていたのでそれは直してあります)

public ArrayList<Card> checkSameCards(Player p) {
    ArrayList<Card> cards = p.getCardHand().getCards();
    ArrayList<Card> discards = p.getCardHand().getCards();
    for(Card c1: cards) {
        for(Card c2: cards) {//エラーはこの行に出てます
            if(c1.getNumber() == c2.getNumber()) {
                if(c1 != c2) {
                    discards.add(c1);
                    discards.add(c2);
                }
            }
        }
    }
    return discards;
}


本サイトではコードを質問文に張り付けただけではちゃんと字下げされません。マークダウンと呼ばれる簡易構文を用いる必要があるのです。それをしておられないため、字下げがない見づらいコードになっているのに気づいておられますよね?

本サイトで質問するなら必ず以下を読んで適切にマークダウンを使ってください。さっそく本質問を編集しなおしておくとよいですよ。

https://teratail.com/help#about-markdown


追記:3つのカードが全て削除されてしまう問題に対して。

元のループアルゴリズムですと同一のリストに対する単純な二重ループですので、同じダイヤの4でそれぞれ異なるインスタンスが3つありそれらをD4#1,D4#2,D4#3とすると
D4#1 vs D4#2 => この2つをdiscardsに追加
D4#1 vs D4#3 => この2つもdiscardsに追加
...
のように、「同一のカードインスタンスを複数回discardsに格納し得る論理」になっている点がまずいですね。
対処には色々方法が考えられれます。discardsへ追加する条件に、「c1もc2もdiscardsに含まれない」を追加するというのが最も単純と思います。含まれるかどうかをチェックするメソッドとしてコレクション一般にcontainsというのがありますので、D4#1.equals(D4#2)がfalseとなるようにCardクラスが定義されているならそれを使えます。D4#1.equals(D4#2)がtrueになるようでしたらcontainsは使えませんので別途自前でcontainsCardメソッドのようなものを定義する必要がでてきます。

投稿

編集

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

  • 2018/02/01 13:11 編集

    ご指摘の方ありがとうございます。初めてで不慣れなため使い方を十分に承知しておりませんでした。申し訳ありません。質問内容は見やすいように訂正致しました。プログラムに関しては訂正すると実行はできました。ありがとうございます。ただ追加の質問で申し訳ないのですが、現状のプログラムを実行すると例えば手札に[3 3 3]と3枚同じカードがあった場合も全てdiscardに格納されてしまいます。もし差し支えなければご回答の方よろしくおねがいいたします。追加の質問の場合質問を編集するのかコメントに返すのかわからなかったのでコメントに返させていただきました。間違っていたら申し訳ないです。

    以下はプログラムの実行結果です(3枚の4が全て消えてしまっています)
    ------------現在の手札を表示します.-----------
    1番目のカード:クラブ8
    2番目のカード:スペード3
    3番目のカード:ダイヤJ
    4番目のカード:スペードJ
    5番目のカード:スペード2
    6番目のカード:クラブ2
    7番目のカード:ダイヤQ
    8番目のカード:ハート9
    9番目のカード:ダイヤ4
    10番目のカード:スペードQ
    11番目のカード:ハートA
    12番目のカード:ハートK
    13番目のカード:スペード4
    14番目のカード:ダイヤK
    15番目のカード:スペード6
    16番目のカード:ハート4
    17番目のカード:クラブ6
    18番目のカード:クラブ5
    ------------ここまで-----------
    ------------現在の手札を表示します.-----------
    1番目のカード:クラブ8
    2番目のカード:スペード3
    3番目のカード:ハート9
    4番目のカード:ハートA
    5番目のカード:クラブ5
    ------------ここまで-----------

    -------------------------------追記-------------------------------------
    上記の方法で訂正したところうまくいきました。ありがとうございます。

    キャンセル

+1

追記: 2018/08/15

怒涛の低評価が入ったので、古い話題ではありますがせっかくなので追記しておきます。

個人的には、CardHandクラスにrankTableなどのフィールドを用意すると楽なように思います。
rankTableは『カードのランク』対『枚数』の連想配列です。

rankTableを適宜確認し、値が2以上のときに整理すれば良いです。

class CardHand {
    private Map<Integer, Integer> rankTable = new HashMap<>();
    {
        IntStream.rangeClosed(1, 13)
            .forEach(e -> rankTable.put(e, 0))
        ;
    }
    public void addCard(Card card) {
        手札に追加する通常の処理;

        rankTable.compute(
            card.getRank(), (key, old) -> old + 1
        );
    }
    public void removeCard(Card card) {
        手札から排除する通常の処理;

        rankTable.compute(
            card.getRank(), (key, old) -> old - 1
        );
    }
    private void removeCardByRank(int rank) {
        Card card = 当該ランクのカードを一枚選択;
        removeCard(card);
    }

    private boolean hasDuplicateCards() {
        return rankTable.entrySet().stream()
            .map(Map.Entry::getValue)
            .anyMatch(v -> 2 <= v)
        ;
    }
    private void dispose() {
        while(hasDuplicateCards()) {
            rankTable.entrySet().stream()
                .filter(entry -> entry.getValue() >= 2)
                .forEach(entry -> {
                    int rank = entry.getKey();

                    for(int i = 0; i < 2; ++i) {
                        removeCardByRank(rank);
                    }
                })
            ;
        }
    }

    その他、必要なメソッド
}

疑似コードなのでこのままでは動きませんが。悪しからず。


ところで、半年以上前の回答に急にマイナスが付くのは異常に思えます。
確かに正確な回答ではありませんでしたが、今日一日で6つもマイナスが付いています。

この件は運営に連絡し、何か異常な動向が無いか調査していただくことにします。

2018/1/30 までの回答

なんでマイナス?と思ってしばらく考えていましたが、
よく考えたら要求は重複する要素の排除じゃないですね。失礼しました。

残骸

重複の除去のためによく使われるのが、一度集合に置き換える方法です。
リスト ⇒ 集合 ⇒ リスト のように変換すれば重複する要素を取り除けます。

『java list 重複 削除』とググればいろいろ出てきますよ。
例えば【3分Java入門】Listで重複するデータを取り除く方法など。

投稿

編集

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

  • 2018/02/01 13:12

    ご回答ありがとうございます。

    キャンセル

  • 2018/08/16 11:37

    ねらわれてる?

    キャンセル

  • 2018/08/16 14:17

    複数アカウントで一斉にマイナス投票するいたずらが以前にもありましたので、いちおう調査をお願いすることにしました。
    あるいは掲示板とかSNSとかで晒されてしまったとか?この場合は正当な評価だとして捉えねばなりませんね、若干虚しいですが。

    キャンセル

0

Listを使ってペアを消そうと思うと、早い番号から消すと後の番号がずれるという問題もあるため、
Mapを使ってペアになったら捨てるという考え方も可能。
Cardの仕様がわかりませんが、カードの番号をStringで決めているなら

Map<String, Card> singles = new HashMap<>();
for (Card c : cards) {
    singles.compute(c.number(), (k, v) -> v == null ? c, null);
}
cardHand = new ArrayList<Card>(singles.values());

投稿

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

15分調べてもわからないことは、teratailで質問しよう!

  • ただいまの回答率 88.20%
  • 質問をまとめることで、思考を整理して素早く解決
  • テンプレート機能で、簡単に質問をまとめられる

関連した質問

同じタグがついた質問を見る