3

I got stuck in very basic way of pushing sequence of Integer-array to arraylist. I am trying alter the solution of polygenelubricants for the question K Combinations of set of Integers such that instead of printing them, I am pushing them to an array list.

My code:

public class Test {

    static ArrayList<String> combinations;
    public static void main(String args[]) {
        Integer[] a3 = { 1, 2, 3, 4, 5 };
        comb(a3, 2);
    }
    public static void comb(Integer[] items, int k) {
        Arrays.sort(items);
        combinations = new ArrayList<String>();
        ArrayList<String> c1 = new ArrayList<String>();
        c1 = kcomb(items, 0, k, new Integer[k]);
        System.out.println("from comb");
        for (String x : c1) {
            System.out.println(x);
        }
    }
    public static ArrayList<String> kcomb(Integer[] items, int n, int k,
            Integer[] arr) {
        if (k == 0) {
            combinations.add(Arrays.toString(arr));
        } else {
            for (int i = n; i <= items.length - k; i++) {
                arr[arr.length - k] = items[i];
                kcomb(items, i + 1, k - 1, arr);
            }
        }
        return combinations;
    }
}

Output:

from comb
[1, 2]
[1, 3]
[1, 4]
[1, 5]
[2, 3]
[2, 4]
[2, 5]
[3, 4]
[3, 5]
[4, 5]

But When I changed the type of ArrayList from String to Integer[] as follows, I am getting redundant output.

Changed Code

public class Test {

    static ArrayList<Integer[]> combinations;
    public static void main(String args[]) {
        Integer[] a3 = { 1, 2, 3, 4, 5 };
        comb(a3, 2);
    }
    public static void comb(Integer[] items, int k) {
        Arrays.sort(items);
        combinations = new ArrayList<Integer[]>();
        ArrayList<Integer[]> c1 = new ArrayList<Integer[]>();
        c1 = kcomb(items, 0, k, new Integer[k]);
        System.out.println("from comb");
        for (Integer[] x : c1) {
            System.out.println(Arrays.toString(x));
        }
    }
    public static ArrayList<Integer[]> kcomb(Integer[] items, int n, int k,
            Integer[] arr) {
        if (k == 0) {
            combinations.add(arr);
        } else {
            for (int i = n; i <= items.length - k; i++) {
                arr[arr.length - k] = items[i];
                kcomb(items, i + 1, k - 1, arr);
            }
        }
        return combinations;
    }
}

Output:

from comb
[4, 5]
[4, 5]
[4, 5]
[4, 5]
[4, 5]
[4, 5]
[4, 5]
[4, 5]
[4, 5]
[4, 5]

Can someone help me to point out what am I doing wrong...

Thanks, Sarath

2
  • are u sure that its ArrayList<Integer[]> not ArrayList<Integer> Commented Jun 23, 2013 at 4:06
  • @codeMan, as we get combinations of integers in a3 it should be ArrayList<Integer[]>. what I am trying to store in arraylist is multiple sets of combinations. Commented Jun 23, 2013 at 4:14

3 Answers 3

2
public class Test {

    static ArrayList<Integer[]> combinations;
    public static void main(String args[]) {
        Integer[] a3 = { 1, 2, 3, 4, 5 };
        comb(a3, 2);
    }
    public static void comb(Integer[] items, int k) {
        Arrays.sort(items);
        combinations = new ArrayList<Integer[]>();
        ArrayList<Integer[]> c1 = new ArrayList<Integer[]>();
        c1 = kcomb(items, 0, k, new Integer[k]);
        System.out.println("from comb");
        for (Integer[] x : c1) {
            System.out.println(Arrays.toString(x));
        }
    }
    public static ArrayList<Integer[]> kcomb(Integer[] items, int n, int k,
            Integer[] arr) {
        if (k == 0) {
            combinations.add(arr);
        } else {
            for (int i = n; i <= items.length - k; i++) {
                Integer[] arr1 = new Integer[arr.length];
                System.arraycopy(arr, 0, arr1, 0, arr.length);
                arr1[arr.length - k] = items[i];
                kcomb(items, i + 1, k - 1, arr1);
            }
        }
        return combinations;
    }
}
Sign up to request clarification or add additional context in comments.

Comments

1

There is your error: combinations.add(arr); you are working always on the same array arr.

Keep in mind, arrays are objects and have a reference. You save the same array arr reference to the ArrayList and keep changing the array values afterwards. You always get the value of the last combination for all other combinations as you are working on the same array every time.

Therefore, you need to clone arr before you add it to the ArrayList in order to obtain a new reference. The code was working before as every String has its own reference as Strings are immutable.

1 Comment

Thanks. Changing combinations.add(arr); to combinations.add((Integer[])arr.clone());
1

the problem is that you are only ever creating one Integer[] array - it is being reused for each call to kcomb, so at the end of the process the same array has been added multiple times to the list, but the contents of the array is only the last combination. Also, you don't need to use Integer[] for this purpose - int[] is quite satisfactory and more efficient.

Comments

Your Answer

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.