Java Generics and ArrayLists

Question

This abomination of code creates arrays based off input from the user. Sample input would be something like "S 4 aba bab baa aab" where:

• args[0] = array type (Int, Double, String)
• args[1] = length of array
• args[2...] = contents of the array

The creation of the arrays in my code is horrible, but I am not quite sure how else to do it while incorporating generics. The point of the code is to be able to pass any of the created arrays to the methods at the bottom, hence generics.

import java.io.FileNotFoundException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashSet;

public class Prog7a<E> {
public static void main(String[] args) throws FileNotFoundException {

    checkUsage(args);

    //This would be nice, but I get "Cannot make static reference to non-static E"
    //      ArrayList<E> array = new ArrayList<>();
    //
    //      if (args[0].matches("I|i")) 
    //          for (int i = 2; i < args.length; i++) 
    //              array.add((E) new Integer(args[i]));
    //
    //       else if (args[0].matches("S|s")) 
    //           for (int i = 2; i < args.length; i++) 
    //                  array.add((E) new String(args[i]));
    //       else 
    //           for (int i = 2; i < args.length; i++) 
    //                  array.add((E) new Double(args[i]));
    //
    //
    //      System.out.println("Original: " + array);
    //      System.out.println("  Unique: " + removeDuplicates(array));
    //      shuffle(array);
    //      System.out.println("Shuffled: " + array);
    //      System.out.println(" Maximum: " + max(array));

My question is here: I have the loop to fill in the array and call to the methods in each conditional, which makes me think I could rewrite this in such a way that it is not so repetitive.

    if (args[0].matches("I|i")) {
        ArrayList<Integer> iL = new ArrayList<Integer>();
        for (int i = 2; i < args.length; i++) {
            iL.add(Integer.parseInt(args[i]));
        }
        System.out.println("Original: " + iL);
        System.out.println("  Unique: " + removeDuplicates(iL));
        shuffle(iL);
        System.out.println("Shuffled: " + iL);
        System.out.println(" Maximum: " + max(iL));

    } else if (args[0].matches("S|s")) {
        ArrayList<String> sL = new ArrayList<String>();
        for (int i = 2; i < args.length; i++) {
            sL.add(args[i]);
        }
        System.out.println("Original: " + sL);
        System.out.println("  Unique: " + removeDuplicates(sL));
        shuffle(sL);
        System.out.println("Shuffled: " + sL);
        System.out.println(" Maximum: " + max(sL));
    } else {
        ArrayList<Double> dL = new ArrayList<Double>();
        for (int i = 2; i < args.length; i++) {
            dL.add(Double.parseDouble(args[i]));
        }
        System.out.println("Original: " + dL);
        System.out.println("  Unique: " + removeDuplicates(dL));
        shuffle(dL);
        System.out.println("Shuffled: " + dL);
        System.out.println(" Maximum: " + max(dL));
    }

}

public static <E> ArrayList<E> removeDuplicates(ArrayList<E> list) {
    new HashSet<E>(list);
    list = new ArrayList<E>(new HashSet<E>(list));              
    return list;

}

public static <E> void shuffle(ArrayList<E> list) {
    Collections.shuffle(list);
}

public static <E extends Comparable<E>> E max(ArrayList<E> list) {
    return Collections.max(list);
}

public static void checkUsage(String[] args)
{
    if (args.length < 2) 
    {
        System.out.println("Please supply more than 2 arguments");
        System.exit(1);
    }
    if (!(args[0]=="s") && (args[0]=="S") &&
        (args[0]=="d") && (args[0]=="D") &&
        (args[0]=="i") && (args[0]=="I")){
        System.out.println("Incorrect format of command.");
        System.exit(1);
    }
}
}

Answer 1

There are several problems with your code. I will start with the method checkUsage(String[]). The check for the number of arguments should be if (args.length <= 2), with a <= instead of a < operator (or, alternatively, if (args.length < 3). Then, the check of the arguments themselves is faulty, because the condition you specify for an incorrect format will never evaluate to true. To check if none of two conditions hold true, you must either test

• if (!(condition1 || condition2))

or

• if (!condition1 && !condition2).

The two are equivalent, but be aware of operator precedence: The unary operator ! will be evaluated before || or &&, so in the first case, you have to surround the expression condition1 || condition2 with parentheses.

Next, you are comparing Strings using ==. This is dangerous, because == only checks for reference equality but not the contents of the Strings. You should rather use String.equals(Object) to compare Strings. In your case, you could even take advantage of String.equalsIgnoreCase(String), halving the number of method invocations and making the code easier to read.

So much for the method checkUsage(String[]). On to the methods shuffle(ArrayList) and max(ArrayList). I don't really see a point in defining these methods, since they do nothing but pass the argument to the respective method in Collections, so you might as well call Collections.max(Collection) and Collections.shuffle(List) directly from main(String[]).

The method removeDuplicates(ArrayList) unnecessarily creates a HashSet in the first line that will never be used, because the expression new HashSet<E>(list) passed to the ArrayList constructor in the second line creates a new HashSet that has nothing to do with the HashSet created in the first line, so the first line is completely redundant. Also be aware that HashSet.iterator() returns the elements contained in the Set in no particular order, so your method removeDuplicates(ArrayList) might not only remove duplicates, but also change the order of the List in general. Furthermore, you might want to consider defining the method parameter as a List (which is an interface) instead of an ArrayList (which is a class), because the method doesn't depend on the specific implementation of the passed List but only on its behavior, which is defined by the interface. That way, the method can also accept other implementations of the List interface and not only ArrayLists. And lastly, the name "removeDuplicates" is a bit misleading, since, unlike shuffle(List), the method doesn't actually modify the passed List but returns a new List without duplicates, leaving the passed List unmodified. Maybe "withoutDuplicates" would be better, or something like that.

Now, about the generic type parameter E. There is no point in defining it in the class declaration because the class itself does not depend on it, only your methods that accept an ArrayList (or List) as a parameter do. I suggest you remove it, because defining the class Prog7a with a generic type parameter <E> and then declaring methods that define a generic type parameter <E> again introduces ambiguity:

public class Prog7a<E> { /* If we instantiate a Prog7a<String>,
                            this E would be String */

    /* Now comes a method declaration that defines a generic
    type parameter <E> again. This newly defined parameter
    hides the parameter defined in the class declaration and is
    completely independent of it, so this method might
    very well accept a List<Integer>, even though the type
    of this class is Prog7a<String> */
    public <E> ArrayList<E> removeDuplicates(List<E> list) {
        // ...
    }
}

Regarding the actual problem at hand, which is the code repetition in your original code, the other two suggested solutions both seem appropriate to me, but I don't get the organization of slowy's code at all. Not only that the execution logic of the program is put inside a constructor, but the constructor actually doesn't serve any other purpose at all except establishing a type parameter, and the generic type parameter of the Prog7a class is unnecessary, as I already explained. This also means that your static methods can, indeed, remain static.

Answer 2

I doubt what you're trying to do is possible. The problem is that the compiler needs to know at compile time what the E is going to be. So as long as you're inside one of your if(...) blocks, the compiler knows what type it is.

The only thing I could think of that you can do is write another helper method to handle the common actions:

private static <E extends Comparable> void print(ArrayList<E> iL) {
    System.out.println("Original: " + iL);
    System.out.println("  Unique: " + removeDuplicates(iL));
    shuffle(iL);
    System.out.println("Shuffled: " + iL);
    System.out.println(" Maximum: " + max(iL));
}

This works for the same reason that the methods shuffle, max and removeDuplicates work.

Answer 3

We actually do know what the type is, not 'compiler-time-ish', but runtime-ish...

for a heads up: I'm really bad at generics and I'm not sure if that's either a bit of a misuse of generics or if it's way too complicated (or could be done much easier)... (edit: and someone should review my posted piece of code, too ...)

I just introduced a TypeParser interface to support the different types (Integer, Double, String) and made the static methods instance methods (so they do bind to the type, which was the main problem, I think) and I'm sure there's room for improvement either way,...

public class Prog7a<E extends Comparable<? super E>> {

    private interface TypeParser<T> {
        T parse(String value);
    }

    public Prog7a(TypeParser<E> typeParser, String... args) {
        List<E> list = new ArrayList<>();
        for (int i = 2; i < args.length; i++) {
            list.add(typeParser.parse(args[i]));
        }
        System.out.println("Original: " + list);
        System.out.println("  Unique: " + removeDuplicates(list));
        shuffle(list);
        System.out.println("Shuffled: " + list);
        System.out.println(" Maximum: " + max(list));
    }

    public List<E> removeDuplicates(List<E> list) {
        new HashSet<>(list);
        list = new ArrayList<>(new HashSet<>(list));
        return list;

    }

    public void shuffle(List<E> list) {
        Collections.shuffle(list);
    }

    public E max(List<E> list) {
        return Collections.max(list);
    }

    public static void main(String[] args) throws FileNotFoundException {
        checkUsage(args);

        if (args[0].matches("I|i")) {
            new Prog7a<>(new TypeParser<Integer>() {

                @Override
                public Integer parse(String value) {
                    return Integer.valueOf(value);
                }
            }, args);
        } else if (args[0].matches("S|s")) {
            new Prog7a<>(new TypeParser<String>() {

                @Override
                public String parse(String value) {
                    return value;
                }
            }, args);
        } else {
            new Prog7a<>(new TypeParser<Double>() {

                @Override
                public Double parse(String value) {
                    return Double.valueOf(value);
                }
            }, args);
        }
    }

    public static void checkUsage(String[] args) {
        if (args.length < 2) {
            System.out.println("Please supply more than 2 arguments");
            System.exit(1);
        }
        if (!(args[0] == "s") && (args[0] == "S") && (args[0] == "d") && (args[0] == "D") && (args[0] == "i") && (args[0] == "I")) {
            System.out.println("Incorrect format of command.");
            System.exit(1);
        }
    }

}

that's the output

Original: [1, 2, 3, 4, 5, 4, 3, 2, 1]
  Unique: [1, 2, 3, 4, 5]
Shuffled: [3, 4, 1, 3, 2, 1, 2, 5, 4]
 Maximum: 5