重複しないランダム配列ゲーム

ドットインストール様のサイトを活用させていただきまして、宝箱ゲームを作っています。
ドットインストール様の動画では、箱をクリックすると、「空」、「罠」、「宝」の３種類がランダムに出てくる仕様になっていますが、
それをアレンジして、３つの宝の中身が決して被らないようにしたいと思っています。
オリジナル）宝、罠、罠など重複あり。　目標）罠、空、宝　など被らない。

実際にその目標通り重複無しのランダムは、以下のようにしたら自分で実装できました。しかし、何となく見栄えが悪い気がしてなりません。shuffle処理してから、シャッフル後の配列の０番目を箱の０番目、シャッフル後の１番目を１番目〜。

上を目指したコーディングを行いたいときに、もっといい書き方がある、などアドバイスを頂けると助かります。よろしくお願いいたします。

HTML

<!DOCTYPE html>
<html lang="ja">
<head>
<meta charset="utf-8">
<title>Treasure Hunt</title>
<link rel="stylesheet" href="css/styles.css">
</head>
<body>
  <div class="container">
    <div class="boxes">
      <img src="img/box.png" width="85" height="100" class="box shake"></img>
      <img src="img/box.png" width="85" height="100" class="box shake"></img>
      <img src="img/box.png" width="85" height="100" class="box shake"></img>
    </div>
  </div>

<script src="js/main.js"></script>
</body>
</html>

Javascript

(function() {
  'use strict';

  var boxes = document.getElementsByClassName('box');

  const contents = [
    'coin.png',
    'empty.png',
    'cobra.png',
  ];

  function shuffle(arr){
    let i = arr.length - 1;
    for(i = arr.length - 1; i > 0; i--){
      const j = Math.floor(Math.random() * (i + 1));
      [arr[j], arr[i]] = [arr[i], arr[j]];
    }
    return arr;
  }

  let mixedContents = shuffle(contents);

  function init() {
    let i;
    for (i = 0; i < boxes.length; i++) {
      boxes[i].addEventListener('click', function() {
        let j;
        for (j = 0; j < boxes.length; j++){
          boxes[0].src = 'img/' + mixedContents[0];
          boxes[1].src = 'img/' + mixedContents[1];
          boxes[2].src = 'img/' + mixedContents[2];
          // boxes[j].src = 'img/' + contents[Math.floor(Math.random() * contents.length)];
          boxes[j].className = 'box disabled';
        }
        this.className = 'box';
      });
    }
   }

  init();
})();

think49

2019/05/02 03:46

"しかし、何となく見栄えが悪い気がしてなりません。" もっと自分で質問内容を咀嚼して、具体的にすべきと思います。「何となく」から、「どうして見栄えが悪い気がするのか」を突き詰めて具体化してみましょう。 https://teratail.com/help/question-tips#questionTips2 回答者からリファクタリングされたコードが掲示されるかもしれませんが、自分で考える機会を失うのは勿体ないと私は思います。

williamsArk

2019/05/02 06:19

ご指摘ありがとうございました。確かにその通りなんですよね。ただとっかかりがわからなかったため、質問してしまいました。他の方のアドバイスを元にしながらも、自分なりのやり方をさらに探求したいと思います。

think49

2019/05/02 07:13

otolabさんが指摘されたようにこれはアルゴリズムの問題ですので、「自分の中でいかにアルゴリズムを描けるか」が鍵となります。アルゴリズム上の無駄を排除することで見栄えの問題はクリアされますので、フローチャートを書いて整理する事をお勧めします。なお、あるアルゴリズムを一つの関数でおさめると非常にすっきりして見えますが、関数内部のアルゴリズムを把握して使わないとブラックボックスが増えるばかりでむしろ、理解度は下がります。頂いたコードの「書き方」だけを覚えるやり方はしない方が良いと私は考えます。

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

回答4件

基本的な発想でいうと、３種類のものを３つ重複なくランダムに並べる問題なので、この方法で悪くないと思います。

ただ、shuffleが本当に入れ替えているところは処理が冗長になると思います。
入れ替えによるシャッフルの確率が本当に均等かも調べたほうが良いかも？

最初からランダムな抽選を三回行うとか、並んで入った箱から次の空箱に順不同で入れるなどの手法がすぐ思いつくところでしょうか。

次は「ｎ種類のものを３つ重複なくランダムに」並べる方法に拡張できる方法を考えると、なんとなく嬉しいかもしれませんね。

投稿2019/05/02 04:32

otolab

総合スコア765

otolab

2019/05/02 04:33

ちなみにこれは「アルゴリズム」に関する質問だと思うので、タグはそういうものをつけたほうが良いと思います。

otolab

2019/05/02 04:51

あとそうだ。「0から2までの数字をランダムに並べる」という問題だとシンプル化しておくと、いろいろ考えやすいと思います。

Lhankor_Mhy

2019/05/02 05:32

横からすみません。アルゴリズム的にはフィッシャーイエーツ法なので、偏りはないと思います。計算量もO(n)なので問題ないかと。 >フィッシャー–イェーツのシャッフルは、全ての順列の組み合わせが等しく存在しうるため、偏りがない。 [フィッシャー–イェーツのシャッフル - Wikipedia](https://ja.wikipedia.org/?curid=3327883)

williamsArk

2019/05/02 06:20

詳しい解説ありがとうございました。色々なやり方がありそうですね。いただいたアドバイスを参考にしながらもう少し追求してみます。感謝いたします。

otolab

2019/05/04 05:42

Lhankor_Mhy さんありがとうございます。不勉強がバレる。。(汗)

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

こんにちは

何となく見栄えが悪い気が

するコードを修正する手段のひとつとして

便利なものがあれば使ってみる

というのがあります。具体的には、ご質問に挙げられているコードでは、

配列をシャッフルする関数を自分で書かないで、Lodash の _.shuffle を使い、
ご質問のタグに JQuery があったので、JQueryも使う

ことにすると、例えば以下のようになります。

html
1<!DOCTYPE html>
2<html lang="ja">
3<head>
4    <meta charset="UTF-8">
5    <title>Q187358</title>
6    <script src="https://cdnjs.cloudflare.com/ajax/libs/lodash.js/4.17.11/lodash.min.js"></script>
7    <script src="https://code.jquery.com/jquery-3.4.1.min.js"></script>
8    <script>
9        $(function(){
10
11          const mixedContents = _.shuffle(['coin', 'empty', 'cobra']);
12
13          const openBoxes = function() {
14            $('.box').each(function(i) {
15              $(this)
16                .attr('src', `img/${mixedContents[i]}.png`)
17                .removeClass('shake')
18                .addClass('disabled');
19            });
20          };
21
22          $('.box').on('click', openBoxes);
23        });
24    </script>
25    <style>
26        .disabled { pointer-events: none; }
27    </style>
28</head>
29<body>
30<div class="container">
31    <div class="boxes">
32        <img src="img/box.png" width="85" height="100" class="box shake" />
33        <img src="img/box.png" width="85" height="100" class="box shake" />
34        <img src="img/box.png" width="85" height="100" class="box shake" />
35    </div>
36</div>
37</body>
38</html>
39

以上、参考になれば幸いです。

投稿2019/05/02 06:11

jun68ykt

総合スコア9058

williamsArk

2019/05/04 12:37

とりあえず新しいものを取り入れて使ってみると言う態度はやはり必要ですよね。基本的な事だとは思うのですが、それの良い再確認になりました。ご協力ありがとうございました。

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

再帰呼び出しを用いた解法です。

JavaScript
1	  function shuffle(arr) {
2		if (arr.length <= 1) {
3			// 配列の要素数が1以下ならば、何もせずそのまま配列を返す。
4			return arr;
5		}
6
7		const i = Math.floor(Math.random() * arr.length);
8		// 配列からインデックスiの要素を1つ取り除き、取り除いた要素(の配列)を戻り値として返す。
9		var deleted = arr.splice(i, 1);
10		// 取り除かれた残りの配列を、再帰呼び出しで並びかえる。
11		arr = shuffle(arr);
12		// 取り除いた要素を最初に、残りの配列を次にして結合した配列を返す。
13		return deleted.concat(arr);
14	  }

投稿2019/05/02 06:09

naomi3

総合スコア1105

williamsArk

2019/05/04 12:36

spliceを使ったやり方も面白そうですね。新しいものはすぐ取り入れてみたいと思います。感謝いたします。

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

私であればこう書く、というレベルの回答になってしまいますが。

js
1    let i = arr.length - 1;
2    for(i = arr.length - 1; i > 0; i--){

↓1行にします。（同様のコードが三箇所あります）

js
1    for(let i = arr.length - 1; i > 0; i--){

js
1        for (j = 0; j < boxes.length; j++){
2          boxes[0].src = 'img/' + mixedContents[0];
3          boxes[1].src = 'img/' + mixedContents[1];
4          boxes[2].src = 'img/' + mixedContents[2];

↓同じことを3回繰り返す必要はなさそうなので。

js
1        for (j = 0; j < boxes.length; j++){
2          boxes[j].src = 'img/' + mixedContents[j];

あとは好みになるかと思いますが、classNameじゃなくてclassListを使うかなあ、とか、thisじゃなくてEvent.targetにして、clickイベントがバブリングしたのを親要素で拾った方が楽かなあ、とか。

投稿2019/05/02 06:06

Lhankor_Mhy

総合スコア36104

williamsArk

2019/05/02 06:26

新たなパターンを教えていただきましてありがとうございました。実はこう言う自分ではやらない（自分が知らない）やり方を期待していました。もう少し自分でも調べろ、って言う話なのかもしれませんがね。 boxes[j].src = 'img/' + mixedContents[j]; でいいのですね。ダブルループ？すると、よくわからなくなってしまうのですが、 iの方のループはクリックしたものに対してのループ、jの方のループはまた違うものを指しているという感じになるのでしょうか？初歩的な質問ですいません。

Lhankor_Mhy

2019/05/02 07:07 編集

これを、ダブルループ（ループのネスト）と呼ぶべきなのかどうか、ちょっと迷う感じですね。個人的にはそう呼びたくないです。たとえば、内側のループですが、j を i に書き換えても全然動くんですよね。 > iの方のループはクリックしたものに対してのループ、jの方のループはまた違うものを指しているという感じになるのでしょうか？うーん……微妙な表現ですね。違う、と言いたくなる感じです。

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

あなたの回答