HiveBrain v1.2.0
Get Started
← Back to all entries
snippetjavaMinor

Generate all permutations

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
permutationsgenerateall

Problem

I have written this Java snippet to generate all permutations of a given array of integers. Can somebody review it and give some pointers as to how to make it more generic? I.e. instead of dealing with only ints, it should be able to handle a collection of any data type.

import java.util.Arrays;
public class PrintPermutations{
        private int[] taperedArray(int[] arr,int idx){
                int nArray[] = new int[arr.length-1];
                for(int i=0,j=0;i<nArray.length;i++,j++){
                  if(j == idx){j+=1;}
                  nArray[i] = arr[j];
                }
                return nArray;
        }

        public void printPermutations(String prefix,int[] arr){
                if(arr.length == 1){
                  System.out.println(prefix + arr[0]);
                }else{
                 for(int i=0;i<arr.length;i++){
                   printPermutations(prefix+arr[i]+",",taperedArray(arr,i));
                 }
                }
        }

        public static void main(String[] args){
                PrintPermutations pp = new PrintPermutations();
                int data[] = new int[]{1,2,3};
                pp.printPermutations("",data);
        }
}

Solution

public class PrintPermutations{


Classes are types of things, so they usually get noun names, e.g.

public class PermutationPrinter {


You mention that you want this to work with generic types. The way to do that is to add a generic parameter.

public void printPermutations(String prefix,int[] arr){


Unfortunately, there are no generic arrays in Java, so you have to switch to a different data type. You have a list of items, so let's switch to List.

public void permuteAndPrint(String prefix, List items) {


Your method actually does two things, not one. Thus, the compound name. This actually violates something called the Single Responsibility Principle, but I'm going to ignore that for now.

I prefer plural names for collections, so I changed the now inaccurate arr to items.

Side note: you use eight column indent for the first two levels and then use smaller indents thereafter. The usual suggestion is to always tab the same amount. I switched to a consistent four column tab, as that's pretty common (standard) in Java programming.

if(arr.length == 1){
                  System.out.println(prefix + arr[0]);
                }else{


Rather than use an if/else construct, it is common to return from the base case of a recursive function:

if (items.size() == 1) {
            System.out.println(prefix + items.get(0));
            return;
        }


This allows the rest of the function to work without extra indent and makes it more recognizable as a base case.

for(int i=0;i<arr.length;i++){
                   printPermutations(prefix+arr[i]+",",taperedArray(arr,i));
                 }


There are several things that I don't like about this, starting with the index-based for loop. I'd rather use the foreach format:

for ( T item : items ) {
            List remnants = new ArrayList(items);
            remnants.remove(item);
            permuteAndPrint(prefix + item + ",", remnants);
        }


A useful feature of List is that it includes a remove function that replaces your taperedArray function.

Now rather than saying arr[i], we can say a more readable item.

So the whole class is now

public class PermutationPrinter {

    public void permuteAndPrint(String prefix, List items) {
        if (items.size() == 1) {
            System.out.println(prefix + items.get(0));
            return;
        }

        for ( T item : items ) {
            List remnants = new ArrayList(items);
            remnants.remove(item);
            permuteAndPrint(prefix + item + ",", remnants);
        }
    }

    public static void main(String[] args) {
        PermutationPrinter printer = new PermutationPrinter();
        List data = Arrays.asList(1,2,3);
        printer.permuteAndPrint("", data);
    }

}


This does mostly the same steps as your original and still violates the Single Responsibility Principle. Normally we'd write main something like

public static void main(String[] args) {
        List data = Arrays.asList(1,2,3);
        PermutationPrinter printer = new PermutationPrinter(data);
        printer.permute();
        printer.print();
    }


This has several advantages. For one, it saves the caller from passing an empty string to the recursive function. Instead, we can now write

public class PermutationPrinter {
    List items;
    List> permutations = new ArrayList<>(); 

    public PermutationPrinter(List data) {
        items = data;
    }

    private void _permute(List permutation, List data) {
        if (data.size()  remnants = new ArrayList(data);
            remnants.remove(datum);
            List elements = new ArrayList(permutation);
            elements.add(datum);
            _permute(elements, remnants);
        }
    }

    private void permute() {
        List permutation = new ArrayList();
        _permute(permutation, items);
    }

    private void print() {
        for ( List permutation : permutations ) {
            T last = permutation.remove(permutation.size() - 1);

            for ( T element : permutation ) {
                System.out.print(element + ", ");
            }

            System.out.println(last);
        }
    }

}


Notice how this hides the details of how the recursive function works inside a public method. This allows the recursive method to be private. We could change its interface tomorrow with no impact outside this class.

Another advantage is that it is easier to unit test the state of variables than output. So we may not be able to unit test print, but we can certainly unit test permute and _permute.

This also allows us to move main out of the class and into its own class. This class shouldn't need a main method.

Note: I haven't attempted to optimize the algorithm. I just rewrote it in a new style.

Code Snippets

public class PrintPermutations{
public class PermutationPrinter<T> {
public void printPermutations(String prefix,int[] arr){
public void permuteAndPrint(String prefix, List<T> items) {
if(arr.length == 1){
                  System.out.println(prefix + arr[0]);
                }else{

Context

StackExchange Code Review Q#84699, answer score: 4

Revisions (0)

No revisions yet.