2
\$\begingroup\$

I have a method that generates a sorted array with random values in a given range. I wanted to make a method that does this but in descending order.

The easiest way to accomplish this is to call the first method that generates a sorted array and then reverse it. I believe the simplest way to reverse the array is to call Collection.reverse() while the array is still an ArrayList.

To accomplish this I added a new method that just generates the ArrayList but it seems a little unnatural and the code is getting messy:

/**
 *
 * @param length length of array to be generated
 * @param minVal minimum value (inclusive) that may be used in array
 * @param maxVal maximum value (inclusive) that may be used in array
 * @return array with random values but in sorted order
 */
public static int[] generateSorted(final int length, final int minVal, final int maxVal)
{
    return randomArrayList(length, minVal, maxVal).stream().mapToInt(i -> i).toArray();
}


public static int[] generateReverseSorted(final int length, final int minVal, final int maxVal)
{
    ArrayList<Integer> data = randomArrayList(length, minVal, maxVal);
    Collections.reverse(data);
    return data.stream().mapToInt(i -> i).toArray();
}

/**
 * 
 * @param length length of arrayList to generate
 * @param minVal minimum value (inclusive) that can appear in list
 * @param maxVal maximum value (inclusive) that can appear in list
 * @return  arrayList containing elements in sorted order
 */
private static ArrayList<Integer> randomArrayList(final int length, final int minVal, final int maxVal)
{
    ArrayList<Integer> data = new ArrayList<>(length);

    for (int i = 0; i < length; i++)
    {
        int rndNum = getRandomVal(minVal, maxVal);
        int insertionPoint = Collections.binarySearch(data, rndNum);
        data.add((insertionPoint > -1 ? insertionPoint : - insertionPoint - 1), rndNum);
    }

    return data;
}

One thing I'm wondering is what exactly is a helper method? I assume it's a small method that helps another with a complex task, so would randomArrayList() be called a helper method? I'm hesitant because I had to offload the majority of the work to it, so now it does more work than the methods that call it do.

The other thing is I'm having trouble knowing how to name the array that is to be filled with random values, I can't think of a better name than data.

Last thing, should the variable data be in the local scope of the method or should it be a member variable of the class? I would never have more than one array I'm using at a time, for example I wouldn't have sorted and reverse sorted array I need to hold on to the values to after I use them once.

How do I make the code cleaner?

\$\endgroup\$
0
6
\$\begingroup\$

The easiest way to accomplish this is to call the first method that generates a sorted array and then reverse it. I believe the simplest way to reverse the array is to call Collection.reverse() while the array is still an Array List.

Instead of sorting first and then reversing later, it's better to combine those two steps:

Collections.sort(list, Collections.reverseOrder());

Sorting efficiently

As already discussed in your previous question, it would be more efficient to populate a list with random values first, and then sort it in a second step. This will actually help cleaning up your current logic, keep reading.

Single responsibility principle

The randomArrayList does 2 things now: it generates a list of random values that is sorted. You could change this to do only one thing: generate a list of random values. Let's rename it generateRandomList.

private static List<Integer> generateRandomList(final int length, final int minVal, final int maxVal) {
    List<Integer> randomList = new ArrayList<>(length);

    for (int i = 0; i < length; i++) {
        int rndNum = getRandomVal(minVal, maxVal);
        randomList.add(rndNum);
    }

    return randomList;
}

Next, generateSorted could use generateRandomList and then sort it. Likewise, generateReverseSorted could also use generateRandomList and then sort it in reverse, as I mentioned in the first point.

public static int[] generateSorted(final int length, final int minVal, final int maxVal) {
    List<Integer> list = generateRandomList(length, minVal, maxVal);
    Collections.sort(list);
    return listToArray(list);
}

public static int[] generateReverseSorted(final int length, final int minVal, final int maxVal) {
    List<Integer> list = generateRandomList(length, minVal, maxVal);
    Collections.sort(list, Collections.reverseOrder());
    return listToArray(list);
}

Is generateRandomList starting to look like a helper method? I think so.

And still sticking to the single responsibility principle, and to reduce copy-paste coding, generateSorted and generateReverseSorted should not have to worry about converting the List to array, that functionality could be extracted to its own method:

private static int[] listToArray(List<Integer> list) {
    return list.stream().mapToInt(i -> i).toArray();
}

Your other questions

The other thing is I'm having trouble knowing how to name the array that is to be filled with random values, I can't think of a better name than data.

I renamed the randomArrayList method to generateRandomList, and it felt quite natural to rename the data variable to randomList.

Last thing, should the variable data be in the local scope of the method or should it be a member variable of the class? I would never have more than one array I'm using at a time, for example I wouldn't have sorted and reverse sorted array I need to hold on to the values to after I use them once.

Local scope, as it is now. I'm not sure if you can gain anything at all by not recreating the ArrayList everytime. The performance difference should be negligible, and unless micro optimization is absolutely critical for your use case, you don't need to worry about it.

Suggested implementation

I'm pasting this here mainly for the unit tests. Before refactoring your methods I wrote the tests to make sure I'm not breaking anything. Having these tests in advance are a real time saver, I recommend you start using them.

class GenerateRandomIntArray {

    private static Random random = new Random();

    public static void setRandom(Random random_) {
        random = random_;
    }

    /**
     * @param length length of array to be generated
     * @param minVal minimum value (inclusive) that may be used in array
     * @param maxVal maximum value (inclusive) that may be used in array
     * @return array with random values but in sorted order
     */
    public static int[] generateSorted(final int length, final int minVal, final int maxVal) {
        List<Integer> list = generateRandomList(length, minVal, maxVal);
        Collections.sort(list);
        return listToArray(list);
    }

    public static int[] generateReverseSorted(final int length, final int minVal, final int maxVal) {
        List<Integer> list = generateRandomList(length, minVal, maxVal);
        Collections.sort(list, Collections.reverseOrder());
        return listToArray(list);
    }

    private static int[] listToArray(List<Integer> list) {
        return list.stream().mapToInt(i -> i).toArray();
    }

    /**
     * @param length length of arrayList to generate
     * @param minVal minimum value (inclusive) that can appear in list
     * @param maxVal maximum value (inclusive) that can appear in list
     * @return arrayList containing elements in sorted order
     */
    private static List<Integer> generateRandomList(final int length, final int minVal, final int maxVal) {
        List<Integer> randomList = new ArrayList<>(length);

        for (int i = 0; i < length; i++) {
            int rndNum = getRandomVal(minVal, maxVal);
            randomList.add(rndNum);
        }

        return randomList;
    }

    private static int getRandomVal(int minVal, int maxVal) {
        return minVal + random.nextInt(maxVal - minVal);
    }
}

public class GenerateRandomIntArrayTest {
    @Test
    public void testSorted() {
        GenerateRandomIntArray.setRandom(new Random(0));
        assertArrayEquals(new int[]{3, 3, 3, 4, 5, 7, 7, 8, 9, 9},
                GenerateRandomIntArray.generateSorted(10, 1, 10));
    }

    @Test
    public void testReverseSorted() {
        GenerateRandomIntArray.setRandom(new Random(0));
        assertArrayEquals(new int[]{9, 9, 8, 7, 7, 5, 4, 3, 3, 3},
                GenerateRandomIntArray.generateReverseSorted(10, 1, 10));
    }
}
\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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