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

前提・実現したいこと

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

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

Exception in thread "main" java.util.ConcurrentModificationException

該当のソースコード

lang
1public ArrayList<Card> checkSameCards(Player p) {
2		ArrayList<Card> cards = p.getCardHand().getCards();
3		ArrayList<Card> discards = p.getCardHand().getCards();
4		for(Card c1: cards) {
5			for(Card c2: cards) {//エラーはこの行に出てます
6		        if(c1.getNumber() == c2.getNumber()) {
7		        	if(c1 != c2) {
8		        	    discards.add(c1);
9		        	    discards.add(c2);
10		        	}
11		        }
12		    }
13		}
14		return discards;
15	}

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

lang
1for(Player p: playerList) {  //実行しているgetGameメソッドの一部
2        	if(p instanceof User) {
3          	  p.getCardHand().showAllCards();
4          	} 
5            ArrayList<Card> cards = judge.checkSameCards(p);
6            for(Card c: cards) {
7            	p.discardCard(c);
8            }
9        }
10        for(Player p: playerList) {
11        	if(p instanceof User) {
12        	  p.getCardHand().showAllCards();
13        	} 	  
14        }

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

lang
1public abstract class Player {  //プレーヤクラス
2	/**　プレーヤー名　*/
3	private String name;
4    /**　手札　*/
5	private CardHand hand = new CardHand();
6	
7	/**
8	 * 名前を指定して、じゃんけんのプレイヤーを作る
9	 * @param name　プレイヤー名
10	 */
11	public Player(String name) {
12		super();
13		this.name = name;
14	}
15
16	/**
17	 * 自分の名前を外部に取得させる
18	 * @return　名前
19	 */
20	public String getName() {
21		return name;
22	}
23	
24	/**
25	 * 自分の手札を外部に取得させる
26	 */
27	public CardHand getCardHand() {
28		return hand;
29	}
30	
31	/**
32	 * 自分の名前をセットする
33	 * @return　セットする名前
34	 */
35	public void setName(String name) {
36		this.name = name;
37	}
38	
39	/**
40	 * 相手の手札から手札を一枚引く
41	 * @return　引いたカード
42	 */
43	public void getCardHand(Player p) {
44		Card c = p.hand.getCardAtRandom();
45		hand.addCard(c);
46	}
47	
48	public void addCardHand(Card c) {
49		hand.addCard(c);
50	}
51	
52    public void discardCard(Card c) {
53    	hand.getCards().remove(c);
54    }
55	
56	/**
57	 * 手札の枚数を表示する
58	 */
59	public void showNumberOfCards() {
60		System.out.println(name+"さんの手札は"+hand.getNumberOfCards()+"枚です．");
61	}
62
63}

行動規範の内容に同意します

回答3件

ベストアンサー

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

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

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

リスト１

java
1for (Card c1: cards) {
2  cards.add(...);
3}

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

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

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

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

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

java
1public ArrayList<Card> checkSameCards(Player p) {
2	ArrayList<Card> cards = p.getCardHand().getCards();
3	ArrayList<Card> discards = p.getCardHand().getCards();
4	for(Card c1: cards) {
5		for(Card c2: cards) {//エラーはこの行に出てます
6	        if(c1.getNumber() == c2.getNumber()) {
7	        	if(c1 != c2) {
8	        	    discards.add(c1);
9	        	    discards.add(c2);
10	        	}
11	        }
12	    }
13	}
14	return discards;
15}

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

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

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

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

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

投稿2018/01/30 12:25

編集2018/02/04 07:38

KSwordOfHaste

総合スコア18394

itchee92

2018/02/06 02:35 編集

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

行動規範の内容に同意します

追記： 2018/08/15

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

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

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

Java
1class CardHand {
2    private Map<Integer, Integer> rankTable = new HashMap<>();
3    {
4        IntStream.rangeClosed(1, 13)
5            .forEach(e -> rankTable.put(e, 0))
6        ;
7    }
8    public void addCard(Card card) {
9        手札に追加する通常の処理;
10
11        rankTable.compute(
12            card.getRank(), (key, old) -> old + 1
13        );
14    }
15    public void removeCard(Card card) {
16        手札から排除する通常の処理;
17
18        rankTable.compute(
19            card.getRank(), (key, old) -> old - 1
20        );
21    }
22    private void removeCardByRank(int rank) {
23        Card card = 当該ランクのカードを一枚選択;
24        removeCard(card);
25    }
26
27    private boolean hasDuplicateCards() {
28        return rankTable.entrySet().stream()
29            .map(Map.Entry::getValue)
30            .anyMatch(v -> 2 <= v)
31        ;
32    }
33    private void dispose() {
34        while(hasDuplicateCards()) {
35            rankTable.entrySet().stream()
36                .filter(entry -> entry.getValue() >= 2)
37                .forEach(entry -> {
38                    int rank = entry.getKey();
39
40                    for(int i = 0; i < 2; ++i) {
41                        removeCardByRank(rank);
42                    }
43                })
44            ;
45        }
46    }
47
48    その他、必要なメソッド
49}

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

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

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

2018/1/30 までの回答

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

残骸

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

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

投稿2018/01/30 11:09

編集2018/08/15 10:44

LouiS0616

総合スコア35660

itchee92

2018/02/01 04:12

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

退会済みユーザー

2018/08/16 02:37

ねらわれてる?

LouiS0616

2018/08/16 05:17

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

行動規範の内容に同意します

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

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