current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

Integer permutation using Heap's algorithm:

import java.util.Arrays;

public class IntegerPermutation {

    private static void swap(Integer[] integerArray, int i, int j) {
        Integer temp = integerArray[i];
        integerArray[i] = integerArray[j];
        integerArray[j] = temp;
    }

    private static void permutationHelper(Integer[] integerArray,
            int currentPosition) {

        if (currentPosition == 1) {
            System.out.println(Arrays.toString(integerArray));
        } else {
            for (int i = 0; i < currentPosition; i++) {
                permutation(integerArray, currentPosition - 1);

                if (currentPosition % 2 == 0) {
                    swap(integerArray, i, currentPosition - 1);
                } else {
                    swap(integerArray, 0, currentPosition - 1);
                }
            }
        }
    }

    public static void permutation(Integer[] integerArray, int lengthOfTheArray) {
        permutationHelper(integerArray, lengthOfTheArray);
    }

    public static void main(String[] args) {

        Integer[] a = new Integer[] { 1, 2, 3 };
        IntegerPermutation.permutation(a, a.length);
    }
}

Can you please critique my code and provide your thoughts on where I should improve my code?

share|improve this question

2 Answers 2

up vote 4 down vote

permutation -> permutationHelper -> permutation -> ...

As @MartinR pointed out, the permutation and permutationHelper functions calling each other are confusing. In fact, permutation is actually pointless, as it does nothing but call permutationHelper.

To give permutation some purpose, you could do this:

  • Make permutationHelper call itself instead of permutation
  • Drop the array length parameter from permutation: it can derive it from the array it receives, and pass that to permutationHelper

Why Integer[] instead of int[] ?

In the posted program, there's really no need for Integer objects. You can work with int[], which is slightly lighter.

Code duplication

There are several duplicated elements in this code:

permutation(integerArray, currentPosition - 1);

if (currentPosition % 2 == 0) {
    swap(integerArray, i, currentPosition - 1);
} else {
    swap(integerArray, 0, currentPosition - 1);
}

First of all, the currentPosition - 1 is duplicated. It would be good to store it in a local variable.

Secondly, swap is called with almost the same parameters, except one. This could be expressed more compactly using a ternary operator.

int nextPosition = currentPosition - 1;
permutation(integerArray, nextPosition);
swap(integerArray, currentPosition % 2 == 0 ? i : 0, nextPosition);

Initializing arrays

A simpler way to initialize arrays is using { ... }, like this:

int[] a = { 1, 2, 3, 4 };
share|improve this answer
    
I am not a Java expert, but isn't Integer[] a = new Integer[] { 1, 2, 3 }; in the original code already the type of initialization which you are suggesting in your last point? –  Martin R Jul 25 at 20:47
    
I forgot to edit after a copy-paste. Fixed now, thanks! –  janos♦ Jul 25 at 20:50
    
"there's really no need for Integer", is it? –  Gerold Broser Jul 27 at 13:07
    
Thanks @GeroldBroser, well spotted –  janos♦ Jul 27 at 13:13
up vote 3 down vote

One thing I noticed is that permutation() calls permutationHelper() with exactly the same arguments. So you don't need two separate methods but just

public static void permutation(Integer[] integerArray,
        int currentPosition) { ... }

which calls itself recursively.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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