Java Generics - Write a generic method to find the maximal element in the range [begin, end] of a list

Question

I'm a Java beginner, going through the Generics Questions and Exercises in Oracle's The Java Tutorials.

Here's my solution to #8:

Write a generic method to find the maximal element in the range [begin, end] of a list.

All suggestions for improvement are welcome. I'd especially like feedback on the following:

• Java library usage
• Algorithm performance
• Exceptions

(imports omitted for brevity)

@SuppressWarnings("serial")
class MaximalElementList<E extends Comparable<E>> extends ArrayList<E> {
 /**
 * @param begin - list index to start iterating from (inclusive)
 * @param end - list index to iterate up to (exclusive)
 * @return max elem in list as determined by the compareTo of the element type.
 */
 public E getMaxInRange(int begin, int end) {
 E max;
 if (isEmpty())
 throw new IllegalStateException("Can't get a max element from an empty list.");
 else {
 max = this.get(begin);
 for (E elem : this.subList(begin, end)) {
 if (elem.compareTo(max) > 0) {
 max = elem;
 }
 }
 }
 return max;
 }
}

Answer 1

So, here's the thing, what you have is not a generic method. What you have is a generic class.

Now, it's a clearly defined generic class, but, it is not what the question asked for. (as a generic class it has a number of issues too, but let's get the method/class issue resolved first).

Generic methods

A generic method is just that, a method, except the parameters (or return value) are of a generic type. A generic method always has a <...> structure before the return-type declaration. A normal method is:

public RType methodName(P1Type p1, P2Type p2, ....) {...}

(where RType is the return type, and P1Type is parameter1 type, etc.).

A generic method has the <...> before the return type, and that construct is used somewhere in the method signature... for example:

public <T> RType methodName(T t, SomeType sparm) {....}

The above is a generic method that has the generic type T as a parameter.

Your Class

So, having stated that your solution is a generic class, not a generic method, let's assume the exercise goal was to produce a generic class. What then?

1. Unless you have exceptional reasons, don't extend ArrayList, "compose" it instead. Have a class that does not extend ArrayList, and have a class field instead, like:

   class MaximalElementList<E extends Comparable<E>> {
    List<E> data = ......
   

2. Your generics here are OK, no problem with <E extends Comparable<E>>

3. Your code should use a guard-condition instead of an else... let me explain. Your code has an if/else:

   public E getMaxInRange(int begin, int end) {
    E max;
    if (isEmpty())
    throw new IllegalStateException("Can't get a max element from an empty list.");
    else {
    max = this.get(begin);
    for (E elem : this.subList(begin, end)) {
    if (elem.compareTo(max) > 0) {
    max = elem;
    }
    }
    }
    return max;
   }
   

   That should instead be:

   public E getMaxInRange(int begin, int end) {
    if (isEmpty()) {
    throw new IllegalStateException("Can't get a max element from an empty list.");
    }
    E max;
    max = this.get(begin);
    for (E elem : this.subList(begin, end)) {
    if (elem.compareTo(max) > 0) {
    max = elem;
    }
    }
    return max;
   }
   

4. Note now, that the max is a messy variable, it can just be:

    E max = this.get(begin);
    for (E elem : this.subList(begin, end)) {
    if (elem.compareTo(max) > 0) {
    max = elem;
    }
    }
    return max;
   

5. Your use of a sublist is smart, but I would consider it to be overkill in this case. How about a simpler implementation:

    E max = this.get(begin);
    for (int i = begin + 1; i < end; i++) {
    if (get(i).compareTo(max) > 0) {
    max = get(i);
    }
    }
    return max;
   

That is now some logic which I think would work well.

Making it a method

Putting that logic in a generic method would "simply" mean:

public <E extends Comparable<E>> E max(List<E> data, int begin, int end) {
 E max = data.get(begin);
 for (int i = begin + 1; i < end; i++) {
 if (data.get(i).compareTo(max) > 0) {
 max = data.get(i);
 }
 }
 return max;
}

With that method, for example, you could do:

List<String> data = Files.readAllLines(Paths.get("some file.txt"));
String maxLine = max(data, 0, 10);

• doughgle: thanks, your answer has been very helpful on the guard condition and cleaning up of the max variable declaration. Could you help further by explaining the reason why I should compose ArrayList instead of extending it?
• doughgle: Also, would you not consider the index variable i in your for loop suggestion to be clutter and the double get(i) to be inefficient?
• rolfl: @doughgle - See the design pattern experts: artima.com/lejava/articles/designprinciples4.html
• rolfl: About the i variable. I happen to know that it is faster to do two get(i) on an array list, than it is to do the sublist of an ArrayList. If you were to be using a linked list, I would change the strategy. But, I did not think this was a hugely significant thing. sublist is relatively seldom used, and can cause confusion.
• rolfl: Also see wikipedia: Composition over inheritance