8
\$\begingroup\$

I've been working on learning about irregular arrays and thought making a generator such as this one would be a good exercise in applying what I have learned.

The following class generates an array containing \$n\$ arrays which themselves contain arrays of integers of decreasing length, of which the maximum length is \$n\$. They are also shuffled to be in random (or, pseudo-random, more accurately) order. The idea behind this is to be able to simply create an array and populate it with data that can be used for developing and testing.

A few general concerns:

  • This is the first toString() I have ever written; how can I improve the code, output, or both, to be more useful or "natural"?

  • I'm still quite new at organizing workflow within a class. My constructor calls a method which chains a few other methods. Is this sensible or terrible design?

  • All other feedback welcomed!

Random2dIntArray.java

import java.util.Random;
/**
 * Create an array of given length which is comprised of inner arrays of random integers.
 */
public class Random2dIntArray {
    int numberOfInnerArrays;
    private int[][] random2dIntArray;
    private Random random = new Random();

    /**
     * Constructor.
     * @param numberOfInnerArrays The number of inner arrays which will be seeded with random integers,
     *                            as well as the length of the longest inner array of random integers.
     */
    public Random2dIntArray(int numberOfInnerArrays) {
        this.numberOfInnerArrays = numberOfInnerArrays;
        this.random2dIntArray = new int[numberOfInnerArrays][];
        generate();
    }
    /**
     * Creates the Random2dIntArray.
     * @return random2dIntArray A shuffled array of arrays of random integers.
     */
    private int[][] generate()  {
        createInnerArrays(random2dIntArray);
        shuffle(random2dIntArray);
        addRandomInts(random2dIntArray);
        return random2dIntArray;
    }
    /**
     * Creates each inner array with a length based on the input parameter numberOfInnerArrays,
     *   which will also be the length of the longest inner array within the outer array.
     * @param randomIrregularArray The outer array based on which the inner arrays will be created.
     */
    private void createInnerArrays(int[][] randomIrregularArray) {
        for (int i = randomIrregularArray.length, j = 0; i > 0; i--, j++) {
            randomIrregularArray[j] = new int[i];
        }
    }
    /**
     * Shuffles the indexes of the array to change their order. Fisher–Yates shuffle array function.
     * inspired by: http://stackoverflow.com/a/18456998/3626537
     * @param sorted2dIntArray The array to be shuffled.
     */
    private void shuffle(int[][] sorted2dIntArray) {
        // Fisher–Yates shuffle array
        // Inspired by: http://stackoverflow.com/a/18456998/3626537
        int randomIndex;
        int[] temp;
        for (int index = sorted2dIntArray.length - 1; index > 0; index--) {
            randomIndex = random.nextInt(index + 1);
            // Swap index values:
            temp = sorted2dIntArray[randomIndex];
            sorted2dIntArray[randomIndex] = sorted2dIntArray[index];
            sorted2dIntArray[index] = temp;
        }
    }
    /**
     * Seeds the inner arrays with random integers.
     * @param randomIrregularArray Iterates over the inner arrays and seeds random integers as values for each index.
     */
    private void addRandomInts(int[][] randomIrregularArray) {
        for (int i = 0; i < randomIrregularArray.length; i++) {
            for (int j = 0; j < randomIrregularArray[i].length; j++) {
                randomIrregularArray[i][j] = random.nextInt();
            }
        }
    }
    /**
     * Renders Random2dIntArray to a String type.
     * @return a String representation of the Random2dIntArray
     */
    public String toString() {
        String arrayAsString = "";
        for (int outerIdx = 0; outerIdx < random2dIntArray.length; outerIdx++) {
            arrayAsString = arrayAsString.concat(String.format("outer[%d]: ", outerIdx));
            for (int innerIdx = 0; innerIdx < random2dIntArray[outerIdx].length; innerIdx++) {
                arrayAsString = arrayAsString.concat(String.format("[%d]: %d ",
                        innerIdx, random2dIntArray[outerIdx][innerIdx]));
            }
            arrayAsString = arrayAsString.concat(System.lineSeparator());
        }
        return arrayAsString;
    }

}

A test call, including printing to console:

public class TestRandom2dIntArray {
    public static void main(String[] args) {
        Random2dIntArray jaggedArray = new Random2dIntArray(5);
        System.out.print(jaggedArray.toString());
    }
}

Sample toString() output:

outer[0]: [0]: 1497180700 [1]: -1267723849 [2]: -1402772085 [3]: -1853139189 [4]: -1840423978 
outer[1]: [0]: 1616095695 [1]: 861133708 [2]: -2108469735 
outer[2]: [0]: -1782952896 [1]: -1979814925 
outer[3]: [0]: 1525848498 [1]: 733271498 [2]: -1194054116 [3]: 553156500 
outer[4]: [0]: 972788909

Another sample toString() to show how the content varies:

outer[0]: [0]: -1537688595 [1]: -1356001667 [2]: 876061606 [3]: -196204872 
outer[1]: [0]: -814279607 
outer[2]: [0]: -933087370 [1]: 1582817382 
outer[3]: [0]: -497198575 [1]: 1270590328 [2]: 553862753 
outer[4]: [0]: -1766682724 [1]: 520775777 [2]: -279377700 [3]: 162768788 [4]: 423268324
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Technically this is not a 2D array, it's a jagged array (or array-of-arrays). \$\endgroup\$ Jan 15, 2016 at 2:31
  • \$\begingroup\$ @EBrown I have updated the title accordingly, however I have not changed the name of the class, so as to not invalidate any potential answer, and give an opportunity to clear up my misunderstanding. \$\endgroup\$
    – Phrancis
    Jan 15, 2016 at 3:22

2 Answers 2

4
\$\begingroup\$

Design

You should let clients pass in their own Random. This lets them get consistent results over multiple passes if they use the same seed.

I like that the class is immutable, but it's odd that there's no way to actually retrieve the values. I would expect you'll add some sort of accessor methods at some point. If so, strongly consider returning copies of the data rather than the original arrays. If you don't, clients can muck with the values in your arrays.

Your class should be final because it is not designed for extension.

Implementation

numberOfInnerArrays is unused and can be tossed.

The generate() method doesn't quite look right. I think you should try to decompose the problem more. Right now you're doing "allocate the arrays, shuffle the arrays, populate the arrays". I think it would be cleaner if the arrays were created and populated, then shuffled.

If you take that approach, you can change your constructor to look more like:

  1. create the array-of-arrays,
  2. for each inner array, invoke a method which returns the inner array.
  3. shuffle the array-of-arrays

createInnerArrays is hard to read with two indexes moving in different directions.

shuffle() should declare randomIndex and temp as final variables inside your loop.

toString() is much more complicated than it needs to be. You should be using a single StringBuilder instance to generate the output string. Since instances are immutable right now, you could conceivably cache the output if it's a performance bottleneck.

Documentation

Your javadoc is, on the whole, decent. You need to talk about error conditions - like if somebody passes in -1 to your constructor.

None of your code comments provide any value, though. You can toss them all.

If you were to apply all these changes, your code might look more like:

import java.util.Random;

public final class Random2dIntArray {

    private final Random random;
    private final int[][] random2dIntArray;

    public Random2dIntArray(final int numberOfInnerArrays) {
        this(numberOfInnerArrays, new Random());
    }

    public Random2dIntArray(final int numberOfInnerArrays, final Random random) {
        if (numberOfInnerArrays < 0) {
            throw new IllegalArgumentException("Cannot have fewer than zero arrays!");
        }

        this.random = random;
        this.random2dIntArray = new int[numberOfInnerArrays][];
        for (int i = 0; i < numberOfInnerArrays; i++) {
            this.random2dIntArray[i] = this.buildInnerArray(i + 1);
        }

        this.shuffle(this.random2dIntArray);
    }

    private int[] buildInnerArray(final int size) {
        final int[] innerArray = new int[size];
        for (int i = 0; i < innerArray.length; i++) {
            innerArray[i] = this.random.nextInt();
        }
        return innerArray;
    }

    /**
     * Shuffles the indexes of the array to change their order. Fisher–Yates shuffle array function.
     * inspired by: http://stackoverflow.com/a/18456998/3626537
     * @param arrayToShuffle The array to be shuffled.
     */
    private void shuffle(final int[][] arrayToShuffle) {
        for (int index = arrayToShuffle.length - 1; index > 0; index--) {
            final int randomIndex = this.random.nextInt(index + 1);
            final int[] temp = arrayToShuffle[randomIndex];
            arrayToShuffle[randomIndex] = arrayToShuffle[index];
            arrayToShuffle[index] = temp;
        }
    }

    /**
     * Renders Random2dIntArray to a String type.
     * @return a String representation of the Random2dIntArray
     */
    @Override
    public String toString() {
        final StringBuilder stringBuilder = new StringBuilder();
        for (int i = 0; i < this.random2dIntArray.length; i++) {
            stringBuilder.append("outer[").append(i).append("]: ");
            for (int j = 0; j < this.random2dIntArray[i].length; j++) {
                stringBuilder.append("[").append(j).append("]: ").append(this.random2dIntArray[i][j]).append(" ");
            }
            stringBuilder.append(System.lineSeparator());
        }
        return stringBuilder.toString();
    }
}
\$\endgroup\$
2
\$\begingroup\$

i think it is better if you make static function to generate irregular array, so you don't need to creat new object every time.

  • createInnerArrays i think you don't need all this parameters in for loop ,you want decrese inner array size every time so it can be like this:

    randomIrregularArray[j] = new int[randomIrregularArray.lenght-j];

and you can fill random numbers in this row,return this row to function (generate) so you can add it direct to your random2dIntArray .

  • shuffle you can drop it, and make list from 0 to numberOfInnerArrays-1 and shuffle it ,when you add row to random2dIntArray in generate pick up random index from the list and add the row in this index so you don't have to loop to shuffle your array.

  • toString i think it's better if you make it general so you can use it with any 2d array , pass 2d array as parameter and return string.

so finally this is the code after edit :

public class Random2dIntArray {

 private  static Random randomForInnerArr;

public static int[][] randomIrregularArray(int numberOfInnerArrays){        
    return generate2dIrregularArray(numberOfInnerArrays);    
    
}

private static int[][] generate2dIrregularArray(int numberOfInnerArrays) {
    randomForInnerArr=new Random();                
    int randomIrregularArray[][]=new int[numberOfInnerArrays][];
    List<Integer> randomList=randomIndexsArray(numberOfInnerArrays);
    for(int i=0;i<numberOfInnerArrays;i++){        
        randomIrregularArray[randomList.get(i)]=createInnerArray(numberOfInnerArrays, i);
    }
    return randomIrregularArray;        
}

private static int[] createInnerArray(int numberOfInnerArrays, int index) {
    int rowSize=numberOfInnerArrays-index;
    int[] row = new int[rowSize];
    for (int j=0;j<rowSize;j++) {
            row[j]=randomForInnerArr.nextInt();
    };
    return row;
}


private static List<Integer> randomIndexsArray(int arrSize){
    List<Integer> randomIndex=new ArrayList<>();
    for(int i=0;i<arrSize;i++){
        randomIndex.add(i);
    }
    Collections.shuffle(randomIndex);
return randomIndex;
}


public static String convertArrayToString(int [][] matrix){    
    String arrayAsString = "";
    int arr2DSize=matrix.length;        
    for (int i = 0; i < arr2DSize; i++) {
        arrayAsString = arrayAsString.concat(String.format("outer[%d]: ", i));
        arrayAsString=arrayAsString.concat(Arrays.toString(matrix[i]));       
        arrayAsString = arrayAsString.concat(System.lineSeparator());
    }
    return arrayAsString;                     
}    
}

Test

public class TestRandom2dIntArray {
public static void main(String[] args) {
    int [][] jaggedArray=Random2dIntArray.randomIrregularArray(5);
    System.out.print(Random2dIntArray.convertArrayToString(jaggedArray));
}
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.