java シャッフルについて

Question

こんいちは。

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

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

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);

Ogre7077 · Accepted Answer

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

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

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

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

Tacosan · Answer

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

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

teketon · Answer

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

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

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

Ogre7077 · Answer

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

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

hanabutako · Answer

#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でも問題ないですね。テストを動かしてみるとそれでもプログラムが壊れないということも確認できると思います。

hanabutako · Answer

> ん～間違っていない気がするのですが。。という思い込みを正すためにテストを書いてみることをおすすめします。実際のコードや動作で確かめれば、プログラムが思ったように動いていないことがわかるでしょう。読み人知らずな格言として、「プログラムは思ったようには動かない。書いた通りに動く」というのがあります。(コンパイラーがバグっていると必ずしもそうではないですが、コンパイラーがバグっている前に自分の書いたコードがバグっていることのほうが普通です) #4のテストをそのまま使えるとは思いますが、リストの中身の型やidの型が何かは全く書かれていないので先の例では適当に書きました。質問者が使っているものが違うならそれに合わせて修正をしてください。ちなみに、#4の回答に書いたテストに質問文のコードのflgまわりのコードを入れると、テストが失敗して間違っていることが一発でわかります。先のプログラムではflgはtrueでシャッフルをもう一度実行する、falseでシャッフルをしないという意味になると思うので、プログラムをそのまま#4の回答で出したneedShuffle関数に入れても問題ないはずです。 public static boolean needShuffle(List 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 list) { HashMap map = new HashMap(); List 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などの統合開発環境も使ったほうが良いです。ソースの整形などは自動でしてくれますし、デバッガーも簡単に動かせますので。

java シャッフルについて

この回答への補足

ループを抜ける判定がおかしいです。

この回答への補足

変数 flg の意味は「先頭8枚に3枚以上の同カードがあれば true」かと思いますが、ロジックでは

この回答への補足

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

この回答への補足

#3 ですが、これは do while 文に対する理解不足が原因の気がしました。

この回答への補足

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

似たような質問が見つかりました

関連するカテゴリからQ&Aを探す

デイリーランキングこのカテゴリの人気デイリーQ&Aランキング

マンスリーランキングこのカテゴリの人気マンスリーQ&Aランキング