patternjavaMinor
Random jagged int array generator
Viewed 0 times
randomarraygeneratorjaggedint
Problem
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
-
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!
```
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 arr
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 arr
Solution
Design
You should let clients pass in their own
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
Implementation
The
If you take that approach, you can change your constructor to look more like:
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:
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:
- create the array-of-arrays,
- for each inner array, invoke a method which returns the inner array.
- 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; 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();
}
}Code Snippets
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();
}
}Context
StackExchange Code Review Q#116841, answer score: 4
Revisions (0)
No revisions yet.