図形配列をシャッフルして別の配列に格納する方法

Question

###前提・実現したいこと

Figure型の配列figures1,2を用意します。
figures1の配列の中身を重複なくランダムにシャッフルしたものをfigures2に格納し、figure2に格納された図形をそれぞれ
図形：面積
という形で出力したいです。

また、可能であればRectangle1,Rectangle2,Circle1,Circle2という風に図形に番号を割り当てたいと考えております。

#求める出力結果
Rectangle1：200
Circle2:78.5
Circle1:1962.5
Rectangle2:600

ここで書いたRectangle,Circle,Circle,Rectangleという順番はあくまで一例なので子の通りでなく、他の順番で出力されてもかまいません。

以下のコードを書いて実行すると、

temp=figures1[index];

のところで、デバッグされ実行できなくなります。

原因が分かる方いらっしゃいましたら、教えていただけると幸いです。
どうぞよろしくお願いいたします。

###ソースコード

import java.util.Random;
import java.util.concurrent.ThreadLocalRandom;

public class Figures {

	public static void main(String[] args) {
		Figure [] figures1 = new Figure[4];
		Figure [] figures2 = new Figure[];
		Figure temp;

        Rectangle r1 = new Rectangle();
		Rectangle r2 = new Rectangle(30,20);
		int i = 0;

        figures1[i++]=r1;
		figures1[i++]=r2;

		Circle c1=new Circle();
		Circle c2=new Circle(25);
        figures1[i++]=c1;
		figures1[i++]=c2;


//ここからのコードの理解が浅いので、間違っている可能性が高いです。
        Random rnd= ThreadLocalRandom.current();
		for(int k=figures1.length-1;k>0;k--) {
			int index=rnd.nextInt(i+1);
			temp = figures1[index];
			figures1[index]=figures1[k];
			figures1[k]=temp;
                }

         for(i=0;i<figures2.length;i++) {


System.out.println(figures2[i].getClass().getName()+":"+figures2[i].getArea());
		}
}


class Figure{
		static int count = 0;
		double area =0;
		double getArea() {
			return 0;
		}
}
class Rectangle extends Figure{

		int width;
		int height;
		String name="Rectangle";

		Rectangle(){
			setSize(10,20);
		}

		Rectangle(int width,int height){
			setSize(width,height);
		}

		void setSize(int w,int h) {
			width=w;
			height=h;
		}

		double getArea() {
			return width*height;
		}
}
class Circle extends Figure{
		int r;
		String name="Circle";
		Circle(){
			setSize(5);
		}
		Circle(int r){
			setSize(r);
		}

		void setSize(int r){
			this.r=r;
		}
		double getArea() {
			return r*r*Math.PI;
		}
}

###借用したコード

上記にて理解が浅いと記載した部分のコードにつきましては、以下のwebページから借用いたしました。
https://blog.y-yuki.net/entry/2018/08/22/094000

import java.util.Arrays;
import java.util.Random;
import java.util.concurrent.ThreadLocalRandom;

public class FisherYatesShuffleSample {

    public static void main(String[] args) {
        int[] targetArray = new int[]{1, 2, 3, 4, 5, 6, 7, 8, 9};

        // シャッフルする前
        printArray(targetArray, "No Shuffle");

        // シャッフル1回目
        shuffle(targetArray);
        printArray(targetArray, "Shuffle 1 ");

        // シャッフル2回目
        shuffle(targetArray);
        printArray(targetArray, "Shuffle 2 ");

        // シャッフル3回目
        shuffle(targetArray);
        printArray(targetArray, "Shuffle 3 ");
    }

    public static void shuffle(int[] array) {
        // 配列が空か１要素ならシャッフルしようがないのので、そのままreturn
        if (array.length <= 1) {
            return;
        }

        // Fisher–Yates shuffle
        Random rnd = ThreadLocalRandom.current();
        for (int i = array.length - 1; i > 0; i--) {
            int index = rnd.nextInt(i + 1);
            // 要素入れ替え(swap)
            int tmp = array[index];
            array[index] = array[i];
            array[i] = tmp;
        }
    }

    private static void printArray(int[] array, String headerComment) {
        System.out.printf("%s -> %s\n\n", headerComment, Arrays.toString(array));
    }
}

Comments

[dodox86]

> 以下のようにコードを書いたところ、うまく結果が出力されませんでした。

「うまく」とはどのようなことなのか、この質問を読んだ者には分かりません。
現状、どのように出力されてしまっているのか。どのように結果が出力されることを望んでいるのかの詳細を、質問文を修正、追記するかたちで示しましょう。

[xebme]

シャッフルを調べ直してください。
int index=rnd.nextInt(i+1); // i+1が怪しい。減っていくはず。k?
https://ja.wikipedia.org/wiki/%E3%83%95%E3%82%A3%E3%83%83%E3%82%B7%E3%83%A3%E3%83%BC%E2%80%93%E3%82%A4%E3%82%A7%E3%83%BC%E3%83%84%E3%81%AE%E3%82%B7%E3%83%A3%E3%83%83%E3%83%95%E3%83%AB

[dodox86]

> ここからのコードの理解が浅いので、間違っている可能性が高いです。
ご自身で書いたコードではないのですか？　コードの意図を理解している訳ではなく、どこからか借用したコード、と言うことなのでしょうか。

[kotton92]

@dodox86さん
https://blog.y-yuki.net/entry/2018/08/22/094000
というページから借用したものになります。
記載が不十分であり、大変失礼いたしました。

[kotton92]

@xebmeさん
承知しました。
ご丁寧にありがとうございます。

[xebme]

要点は２つ。
figures2が初期化されていないのは気づいていますか？
スワップしないでfigures1の要素をfigures2に移送することに気づいていますか？iをkに置き換えたのは移送したいからでしょう。
<--- いい加減なことを言っているかもしれない。回答します。

Answer 1

次のコードは理解できますか？
これを参考に自分のコードを書いてみてください。

import java.util.Random;

public class Figures {
    public static void main(String[] args) {
        Figure[] fig1 = {
			new Rectangle(), new Rectangle(30, 20), new Circle(), new Circle(25)
		};
        Figure[] fig2 = {
			fig1[0], fig1[1], fig1[2], fig1[3] 
		};
        Random rnd = new Random();
        for (int i = 0; i < fig2.length; i++) {
            int j = rnd.nextInt(fig2.length);
            Figure t = fig2[i]; fig2[i] = fig2[j]; fig2[j] = t;
        }
        for (int i = 0; i <fig2.length; i++)
			System.out.println(fig2[i].getName() + ": " + fig2[i].getArea());
	}
}

class Figure {
	static int count = 0;
	double getArea() { return 0; }
	String getName() { return "Figure"; }
}

class Rectangle extends Figure {
	int width;
	int height;
	String name = "Rectangle_" + ++count;

	Rectangle() { setSize(10, 20); }
	Rectangle(int width, int height) { setSize(width, height); } 
	void setSize(int w, int h) { width = w; height = h; }
	double getArea() { return width * height; }
	String getName() { return name; }
}

class Circle extends Figure {
	int r;
	String name = "Circle_" + ++count;

	Circle() { setSize(5); }
	Circle(int r) { setSize(r); }
	void setSize(int r) { this.r = r; }
	double getArea() { return r * r * Math.PI; }
	String getName() { return name; }
}

static int count = 0; がどのように使用されているか、
String と int に +演算子を適用するとどうなるか、
getName() がどういう働きをするのか、などが理解できますか？

Comments

[kotton92]

お返事遅くなってしまい申し訳ございません。
ありがとうございます。参考にさせていただきます。

Answer 2

figure1/figure2の仮定

figure2のインスタンスを用意しているので、figure1はシャッフルしないで元のまま残す、と仮定します。

---

figure2

figure2のインスタンスをどうするか？figure1の要素を全て設定した後で複製するのが良いでしょう。

Figure[] figures2 = figures1.clone();

あとはお手本に従ってfigures2をシャッフルしてください。

シャッフルのバグ

シャッフルのループはiをkに変えたのだから全てkに変えてください。

int index=rnd.nextInt(k+1);

メソッド

複製とシャッフルを行うメソッドを書くとすれば次のようになります。

  static Figure[] cloneShuffle(Figure[] figures) {
    Figure[] result = figures.clone();
    Random rnd = ThreadLocalRandom.current();
    for (int k = result.length - 1; k > 0; k--) {
      int index = rnd.nextInt(k + 1);
      Figure temp = result[index];
      result[index] = result[k];
      result[k] = temp;
    }
    return result;
  }

シャッフルに型は関係ないから型変数を使うと次のようになります。

  static Figure[] cloneShuffle__(Figure[] figures) {
    return cloneShuffle_(figures);
  }
  
  static <T> T[] cloneShuffle_(T[] figures) {
    T[] result = figures.clone();
    Random rnd = ThreadLocalRandom.current();
    for (int k = result.length - 1; k > 0; k--) {
      int index = rnd.nextInt(k + 1);
      T temp = result[index];
      result[index] = result[k];
      result[k] = temp;
    }
    return result;
  }

---

シャッフル別解

質問に対する[追記・修正依頼]で指摘した方法です。これは難しくなります。バグがあるかもしれません。

figure2の初期化。

Figure [] figures2 = new Figure[figures1.length];

シャッフル。

  Random rnd = ThreadLocalRandom.current();
  for (int k = figures1.length; k > 0;) {
    int index = rnd.nextInt(k);
    temp = (figures2[index] == null) ? figures1[index] : figures2[index];
    if (index != --k) {
      figures2[index] = (figures2[k] == null) ? figures1[k] : figures2[k];
    }
    figures2[k] = temp;
  }

---

クラス名にインスタンス番号をつける

カウンターをスレッドセーフにするためにjava.util.concurrent.atomic.AtomicIntegerを使います。int型のcounterを使用すると++counterは２命令になりアトミックではないため。クラス構成にかなり問題がありましたので大幅に手直ししました。

abstract class Figure {

  final String name;

  public Figure(String name) {
    this.name = name;
  }

  abstract double getArea();

  @Override
  public String toString() {
    return name + ":" + String.format("%.1f", getArea());
  }

}

Rectangleクラス

class Rectangle extends Figure {

  static final String NAME = Rectangle.class.getSimpleName();
  static final java.util.concurrent.atomic.AtomicInteger counter = new AtomicInteger(0);
  final int width;
  final int height;

  Rectangle() {
    this(10, 20);
  }

  Rectangle(int width, int height) {
    super(NAME + counter.addAndGet(1));
    this.width = width;
    this.height = height;
  }

  @Override
  double getArea() {
    return width * height;
  }

}

Circleも同様です。

Comments

[xebme]

setterは危険なので意図的に削除しました。コンストラクターの中からオーバーライドされる可能性のあるメソッドは呼んではならない。コンストラクターから呼ぶメソッドはfinalかprivateにすること。

[kotton92]

分かりました。
ご丁寧に教えていただきありがとうございます。