java シャッフルについて

質問

こんいちは。

今、カードゲームを作っているのですが、
リストに格納された先頭の８件に３件以上、重複したものがあったら、
再シャッフルし続けると言うロジックを組んでいるのですが、
ログを見ると、３件以上なのに終了したり、最初の８件に３件以上無いのに
シャッフルしてます。

このロジックに間違いないと思っているんですが、
どこか間違っていますでしょうか？

boolean flg = true;
do {
Collections.shuffle(list, new Random());
flg = true;
for (int i = 0; i < 8; i++) {
int cnt = 0;
for (int j = 0; j < 8; j++) {
if (list.get(i).id.equals(list.get(j).id))
cnt++;
}
if (cnt > 2) {
flg = false;
break;
}
}
} while (flg);

回答No.5

Ogre7077 · ベストアンサー

#3 ですが、これは do while 文に対する理解不足が原因の気がしました。
なので、無限ループの break 終了でロジックを組んでは如何でしょうか。

while (true) {
シャッフル();
int 重複枚数 = 重複を勘定();
if ( 重複枚数 < 3 ) break; // 条件を満たしたのでループ終了
}

この無限ループ記法は、人によっては嫌われることもあります。
その場合は以下の様に誤魔化せば大丈夫でしょう。

for (int i = 0; i < Integer.MAX_VALUE; i++)

補足

ありがとうございます。

ご指摘の通りでした。
多次元ループの場合、breakで一介層のループを抜けると思っていた私のコードミスでした。

うまく通るようになったと思います。
ありがとうございました。

回答No.6

hanabutako

> ん～間違っていない気がするのですが。。

という思い込みを正すためにテストを書いてみることをおすすめします。実際のコードや動作で確かめれば、プログラムが思ったように動いていないことがわかるでしょう。
読み人知らずな格言として、「プログラムは思ったようには動かない。書いた通りに動く」というのがあります。(コンパイラーがバグっていると必ずしもそうではないですが、コンパイラーがバグっている前に自分の書いたコードがバグっていることのほうが普通です)

#4のテストをそのまま使えるとは思いますが、リストの中身の型やidの型が何かは全く書かれていないので先の例では適当に書きました。質問者が使っているものが違うならそれに合わせて修正をしてください。

ちなみに、#4の回答に書いたテストに質問文のコードのflgまわりのコードを入れると、テストが失敗して間違っていることが一発でわかります。先のプログラムではflgはtrueでシャッフルをもう一度実行する、falseでシャッフルをしないという意味になると思うので、プログラムをそのまま#4の回答で出したneedShuffle関数に入れても問題ないはずです。

public static boolean needShuffle(List<Card> list) {
boolean flg = true;
for (int i = 0; i < 8; i++) {
int cnt = 0;
for (int j = 0; j < 8; j++) {
if (list.get(i).id.equals(list.get(j).id))
cnt++;
}
if (cnt > 2) {
flg = false;
break;
}
}
return flg;
}

そして、これを実行すると次のエラーを得ます。
Exception in thread "main" java.lang.Exception: FAILED: no same card should not need shuffle: 0 1 2 3 4 5 6 7 8
at ShuffleTest.assertTrue(ShuffleTest.java:99)
at ShuffleTest.assertFalse(ShuffleTest.java:104)
at ShuffleTest.testNeedShuffle(ShuffleTest.java:110)
at ShuffleTest.main(ShuffleTest.java:167)

つまり、重複しない場合にneedShuffleがtrueを返す、つまり質問者のプログラムのflgはtrueになっているようです。
正しくは、自分の#4の回答のコードで、#3でも回答がついたとおり、flgにtrue / falseを入れるところを逆にします。

あるいは、これが間違っていないとするとあなたが実装したいプログラムというのを自分が根本的に誤解しているかもしれません。そう考えると#4の回答で自分が書いたテストコードも全ては誤りでしょう。書いてあることを実直に書くとしたらこんなコードになると思っていたのですが....
final private static int NUMBER_TO_CHECK = 8;
final private static int MAX_DUPE = 2;
public static boolean needShuffle(List<Card> list) {
HashMap<String,Integer> map = new HashMap<String, Integer>();
List<Card> subList = list.subList(0, NUMBER_TO_CHECK);
for (Card card : subList) { // もし意味が取れないなら"java foreach"で検索
Integer cnt;
if (map.containsKey(card.id)) {
cnt = map.get(card.id);
} else {
cnt = new Integer(0);
}
cnt++;
map.put(card.id, cnt);
}
if (Collections.max(map.values()) > MAX_DUPE) {
return true;
}
return false;
}

リストの最初の8個の要素について、各idの出現回数を調べ、出現回数が2より大きければシャッフルが必要、そうでないならシャッフル不要と判定。
うまい変数名が思いつきませんでしたが、NUMBER_TO_CHECKでリストのどの要素までみるかを示し、MAX_DUPEで許される最大の頻度を示しています。
たった8個の要素でこんなことをしたらやりすぎだと思いますが。

一応、自分が書いたなんちゃってテストは質問に書いてあるシャッフルを再実行するための判定が間違っていることを教えてくれてます。テストが通るようにコードを修正するか、テストの不備を指摘するかしてもらわないとこれ以上の回答はできないです。また、当たり前ですが"ログを見ると、３件以上なのに終了したり、最初の８件に３件以上無いのにシャッフルしてます。"というパターンもテストに追加すると良いと思います。

また、ある程度の規模のプログラムを書くなら各モジュールごとにテストを書くことをおすすめします。JavaだとJUnitがフレームワークとしてよく使われると思います。あと、もし使っていなかったらEclipseやNetBeansなどの統合開発環境も使ったほうが良いです。ソースの整形などは自動でしてくれますし、デバッガーも簡単に動かせますので。

お礼

おっしゃる通りです。

ログで出力してはいたのですが、数字の羅列であれ？と感じながらも横着しました。

正直、テストパターンが思いつかず・・

回答No.4

hanabutako

#3の回答でなんか解決したっぽいですが、一応書いてしまったのでちょっと回答に載せます。

件の判定部分が正しく動いているか気になったので自分は別メソッドにしてそのテストを色々と書いていました。で、テストが上手く動かなかったので無意識的に自分はシャッフルが必要か判定するメソッドを修正してしまい、あなたのプログラムにあった問題には全く気づかなかったわけですが (汗

本来はJUnitなど真面目なフレームワークを使うべきところですが、回答に書いたコードが実行できないのもよくないのでなんちゃってフレームワークを作ってみました。

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;

class Card {
public String id;
}

class CardUtil {
public static boolean needShuffle(List<Card> list) {
boolean flg = false;
for (int i = 0; i < 8; i++) {
int cnt = 0;
for (int j = 0; j < 8; j++) {
if (list.get(i).id.equals(list.get(j).id)) {
cnt++;
}
}
if (cnt > 2) {
flg = true;
break;
}
}
return flg;
}

public static void shuffle(List<Card> list) {
do {
Collections.shuffle(list);
} while (needShuffle(list));
}
}

public class ShuffleTest {
private static List<Card> generateCards(String[] tv) {
List<Card> list = new ArrayList<Card>();
for (int i = 0; i < tv.length; i++) {
Card card = new Card();
card.id = tv[i];
list.add(card);
}
return list;
}

private static String appendAllString(String[] strings) {
StringBuffer buffer = new StringBuffer();
for (int i = 0; i < strings.length; i++) {
buffer.append(strings[i]);
buffer.append(" ");
}
return buffer.toString();
}

private static void assertTrue(boolean bool, String message, String[] strings) throws Exception {
if (!bool) {
StringBuffer buffer = new StringBuffer();
buffer.append("FAILED: ");
buffer.append(message);
buffer.append(": ");
buffer.append(appendAllString(strings));
throw new Exception(buffer.toString());
}
}

private static void assertFalse(boolean bool, String message, String[] strings) throws Exception {
assertTrue(!bool, message, strings);
}

public static void testNeedShuffle() throws Exception {
String[] tv = {"0", "1", "2", "3", "4", "5", "6", "7", "8"};
List<Card> list = generateCards(tv);
assertFalse(
CardUtil.needShuffle(list), "no same card should not need shuffle",
tv);

String[] tv2 = {"0", "0", "2", "3", "4", "5", "6", "7", "8"};
list = generateCards(tv2);
assertFalse(
CardUtil.needShuffle(list), "one pair should not need shuffle",
tv2);

String[] tv3 = {"0", "0", "0", "3", "4", "5", "6", "7", "8"};
list = generateCards(tv3);
assertTrue(
CardUtil.needShuffle(list), "three same card should need shuffle",
tv3);

String[] tv4 = {"0", "0", "0", "0", "4", "5", "6", "7", "8"};
list = generateCards(tv4);
assertTrue(
CardUtil.needShuffle(list), "four same card should need shuffle",
tv4);

String[] tv5 = {"0", "0", "2", "2", "4", "5", "6", "7", "8"};
list = generateCards(tv5);
assertFalse(
CardUtil.needShuffle(list), "two pair should not need shuffle",
tv5);

String[] tv6 = {"0", "0", "2", "2", "2", "5", "6", "7", "8"};
list = generateCards(tv6);
assertTrue(
CardUtil.needShuffle(list), "three same should need shuffle",
tv6);

String[] tv7 = {"0", "0", "2", "2", "4", "4", "6", "7", "8"};
list = generateCards(tv7);
assertFalse(
CardUtil.needShuffle(list), "three pair should not need shuffle",
tv7);

String[] tv8 = {"0", "0", "2", "2", "4", "4", "4", "7", "8"};
list = generateCards(tv8);
assertTrue(
CardUtil.needShuffle(list), "three same should need shuffle",
tv8);

String[] tv_shuffle1 = {"0", "0", "0", "3", "4", "5", "6", "7", "8", "9"};
list = generateCards(tv_shuffle1);
CardUtil.shuffle(list);
assertFalse(
CardUtil.needShuffle(list), "should not need shuffle after shuffle",
tv_shuffle1);

System.out.println("OK");
}

public static void main(String[] args) throws Exception {
testNeedShuffle();
}
}

というわけで、シャッフルが再び必要か判定するルーチンを切り出して、色々なパターンで正しく動くか入れてみるとどこが悪いのか見えてくると思います。

ちなみに、件の判定で2回目のforはj = iでも問題ないですね。テストを動かしてみるとそれでもプログラムが壊れないということも確認できると思います。

補足

ここまでしていただいて、ただただ恐れ入ります。

３番目の方の補足にも書いたのですが、
下記がフラグの切替と判定部です。

(1)３件以上ないものとして、フラグを初期化
(2)３件以上あったのでフラグを倒す
(3)フラグが初期化状態で無いならループをシャッフルからやり直し

j = iはそうですね。おっしゃる通りです。

ん～間違っていない気がするのですが。。
解答いただくのも、言葉で表現しづらいですよね。。
すみません。。

回答No.3

Ogre7077

変数 flg の意味は「先頭8枚に3枚以上の同カードがあれば true」かと思いますが、ロジックでは
3枚未満が true なので再シャッフル
3枚以上が false なので終了
となっています。
なので真偽を逆にすれば解決です。

ところでありがちな失敗談として、変数名に適当な名前を付けたばかりに、
勘違いして変なロジックを組んでしまう、というのがあります。
今回の場合だと flg ではなく tripleOver とでもしていれば、
打鍵している最中にロジックの間違いに気づけたのではないでしょか。

補足

ありがとうございます。

truefalseを逆にするお話ですが、下の(1)(2)を逆にすると(3)で抜けてしまいます。
(3)もはんてんさせると結局ロジックの流れが同じになると思うのですが。。

do {
　Collections.shuffle(list, new Random());
　flg = true;(1)
　for (int i = 0; i < 8; i++) {
　　int cnt = 0;
　　for (int j = 0; j < 8; j++) {
　　　if (list.get(i).id.substring(0,2).equals(list.get(j).id.substring(0,2)))
　　　　cnt++;
　　}
　　if (cnt > 2) {
　　　flg = false;(2)
　　　break;
　　}
　}
} while (flg);(3)

回答No.2

teketon

ループを抜ける判定がおかしいです。パッと見た感じですが、

１．先頭8件の全てを走査していないのに、ループを抜ける判定をしています。
２．cnt>2だと3件以上の重複が在った場合、ループを抜けます。

残念ながら、ロジックが間違っています。

補足

ありがとうございます。

３件以上は抜けて良いのですが。。

１件目のデータから、８件目までループして、その結果３件以上あったら～

再シャッフルして、１件目から再チェックを延々くり返す感じです。

回答No.1

Tacosan

「リストに格納された先頭の８件に３件以上、重複したものがあったら」ってのは, どういう状態なんでしょうか? 具体的に「リストに格納された先頭の８件に３件以上、重複したものがあ」る例, そうでない例をいくつかずつ出してもらえないでしょうか.

cnt の初期化がそこでいいのかどうかよくわからんのですが....

補足

ありがとうございます。

花札をイメージして頂けると分かりやすいと思うのですが、

４枚１２組＝全４８枚　あるカードで

そこで、ゲームスタート時、８枚を場に出すと思います。
そこで、３枚同組のものが出てしまったら、最後までそのぺを誰も取れないので、きりなおしますよね？

そんなロジックを組もうと思ってます。