3
\$\begingroup\$

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.

\$\endgroup\$

2 Answers 2

10
\$\begingroup\$

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).
\$\endgroup\$
4
  • \$\begingroup\$ When changing a passed object reflects tge changes outside of the scope usually prigrammers talk about pass by reference. Java passes eeverything by reference, thusly passes the reference by value. The formulation in your answer is a little unclear about that \$\endgroup\$
    – Vogel612
    Feb 8, 2015 at 12:04
  • \$\begingroup\$ @Vogel612 But Java doesn't pass everything by reference (see eg here (passed into methods by value), or here (When the method [..] is invoked [..], the values of the actual argument expressions initialize newly created parameter variables)). That is why arr = new int[arr.length]; in the swap method would actually not change the array outside of the method. \$\endgroup\$
    – tim
    Feb 8, 2015 at 12:35
  • \$\begingroup\$ But I would agree that objects are passed via references which are passed by value in Java (that is also what I tried to express in my answer). \$\endgroup\$
    – tim
    Feb 8, 2015 at 12:35
  • \$\begingroup\$ About pseudo-random as a source - PRNGs while not truly random, should also not be biased. Randomness and Bias are independent concepts \$\endgroup\$
    – rolfl
    Feb 16, 2015 at 11:39
9
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

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

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