Shuffle the elements of the array randomly

Question

I have written the code to shuffle the elements of the array. Please let me know of any loopholes in the written code.

package dataStructureInJava;

import java.awt.SystemTray;
import java.util.Random;
import java.util.Scanner;

public class ShuffleArr {
    public static void main(String[] args){
        System.out.println("Main Method Started");
        int [] arr = initArr();
        printArr(arr);
        arr = swapArr(arr);
        printArr(arr);
        System.out.println("Main MEthod Ended");
    }
    public static int[] initArr(){
        Scanner in = new Scanner(System.in);
        Random rand = new Random();
        System.out.println("Enter the limit of the arr");
        int limit = in.nextInt();
        System.out.println("Enter the maximum value of the arr");
        int maxValue = in.nextInt();
        int[] arr = new int[limit];
        for(int ii=0;ii<limit;ii++){
            arr[ii] =  rand.nextInt(maxValue);
        }
        return arr;
    }
    public static int[] swapArr(int[] arr){
        Random rand = new Random();
        for(int ii=0;ii<arr.length;ii++){
            arr = swap(arr,ii,rand.nextInt(arr.length));
        }
        return arr;
    }
    public static int[] swap(int[] arr,int ii,int jj){
        int temp=arr[ii];
        arr[ii]=arr[jj];
        arr[jj]=temp;
        return arr;
    }
    public static void printArr(int[] arr){
        System.out.println("Element of the arr are");
        System.out.print("{");
        for(int ii=0;ii<arr.length;ii++){
            System.out.print(arr[ii] + " ");
        }
        System.out.print("}");
        System.out.println();
    }
}

I got his question as an assignment and want to know if it is correct or not.

Answer 1

Naming

• I would write array out instead of shortening it to arr (it's just two more letters), because it's more readable (but I do like that you are very consistent with your shortening).
• is there a reason that you are using ii/jj instead of the more commonly used i/j?

Spacing

You should use a lot more spaces, because they make your code easier to read.

I would at least add spaces after , and ;, before {, and around all = and <.

You should also try to be more consistent with your spacing. For example, you have spaces around some =, but not all, and sometimes you write int [], other times int[].

Randomness

It seems that you implemented a variation of the fisher yates algorithm, which is actually not truly random, but biased:

Similarly, always selecting j [rand.nextInt(arr.length) in your code] from the entire range of valid array indices on every iteration also produces a result which is biased, albeit less obviously so.

You can go to Wikipedia to read the explanation. Implemented correctly, the swapArr method should probably look like this:

public static int[] swapArr(int[] arr) {
    Random random = new Random();
    for (int i = arr.length - 1; i >= 0; i--) {
        int j = random.nextInt(i + 1);
        arr = swap(arr, i, j);
    }
    return arr;
}

Of course, Random only uses pseudorandom numbers anyways, so the slight bias might not be all that important.

Pass by Value

Strictly speaking, Java is pass-by-value, but you could still to this for example (because references of objects are passed, and thus changes to an object affect the object outside of the scope of a method):

public static void swap(int[] arr, int ii, int jj) {
    int temp = arr[ii];
    arr[ii] = arr[jj];
    arr[jj] = temp;
}

And then just call it like this:

swap(arr, i, j);

The same is true for swapArr. I think writing it this way is clearer (and also shorter), but it's probably a matter of taste.

Misc

• I would split initArr into two methods: one that gets the user input, and one that creates the array (that way, it's easier to get the input from different places, for example hardcoded for testing).

Answer 2

Everything is static. This might be fine in this case, but it's bad style in general. And no OOP.

You don't really need to create an object for this assignment, but in case you do, you can make more fancy things. For example, store a Random in a field. This way you can choose

• seeded Random so you want it to always do the same (nice for debugging)
• unseeded Random so you get what you have now
• SecureRandom so you get real randomness

Some factory methods like

static ShuffleArr newSecureShuffleArr() {
     return new ShuffleArr(new SecureRandom());
}

can make it easy to use.

---

package dataStructureInJava; - this should contain the URL of a domain of yours, if you have any; otherwise imagine you do and use something like com.rahulsah.dataStructureInJava.

ShuffleArr - it's a class and you should use a noun describing what it is. Is it an ArrayShuffler?

swapArr(int[] arr) is a bad name. Does it swap? No - see the title. Do we need to say it works with arrays? No - see the arg.

ii, but why? jj? You've saved two letters with arr, so save one here.

---

Random rand = new Random(); - usually, you should create just one instance and stick with it. See above.

---

I agree with everything what tim wrote. Note that both modifying an object and returning it may be pretty confusing.

That all said, it's not bad. There's a clear separation of concerns, which makes the program clear and changes easy.