patternjavaMinor
Heap's algorithm - integer permutation
Viewed 0 times
algorithmpermutationintegerheap
Problem
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
Can you please critique my code and provide your thoughts on where I should improve my code?
`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
Can you please critique my code and provide your thoughts on where I should improve my code?
Solution
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
permutationHelpercall itself instead ofpermutation
- Drop the array length parameter from
permutation: it can derive it from the array it receives, and pass that topermutationHelper
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 };Code Snippets
permutation(integerArray, currentPosition - 1);
if (currentPosition % 2 == 0) {
swap(integerArray, i, currentPosition - 1);
} else {
swap(integerArray, 0, currentPosition - 1);
}int nextPosition = currentPosition - 1;
permutation(integerArray, nextPosition);
swap(integerArray, currentPosition % 2 == 0 ? i : 0, nextPosition);int[] a = { 1, 2, 3, 4 };Context
StackExchange Code Review Q#98071, answer score: 4
Revisions (0)
No revisions yet.