What can we do with generics in Java to make them look better:

Question

I have this method to transform a List to a Map using one of the properties of the elements of the list:

For short it looks like this:

private Map<String, List<Diagnostic<? extends JavaFileObject>>> toMap( List<Diagnostic<? extends JavaFileObject>> diagnostics ) {
    Map<String, List<Diagnostic<? extends JavaFileObject>>> result = new HashMap<String, List<Diagnostic<? extends JavaFileObject>>>();
    for ( Diagnostic<? extends JavaFileObject> d : diagnostics ) {
        List<Diagnostic<? extends JavaFileObject>> list = null;
        if ( !result.containsKey( d.getCode() ) ) {
            list = new ArrayList<Diagnostic<? extends JavaFileObject>>();
            result.put( d.getCode(), list );
        } else {
            list = result.get( d.getCode() );
        }
        assert list != null;
        list.add( d );
    }
    return result;
}

Yiack!..

I like genercis a lot, I use java prior to them and I don't want to go back to the cast everything era, but when a generic contains as element a generic element it self, things go messy.

I know in Java1.7 we will be able to use the "diamond" operator, but there should be another way.

This is what it would look like in a non-generic version:

private Map toMap( List diagnostics ) { 
    Map result = new HashMap();
    for( Object o  : diagnostics ) {
        Diagnostic d = ( Diagnostic ) o; 
        List list = null;
        if( !result.containsKey( d.getCode() ) ) { 
            list = new ArrayList();
            result.put( d.getCode() , list );
         } else { 
            list = result.get( d.getCode() );
         }
         assert list != null;
         list.add( d );
     }
     return result;
}

_{Approximately, I didn't try to compile it.}

How other languages handle this? C# for instance?, Scala? I liked a lot the way SML or Haskell do handle, but something I think too much magic may hurt ( but this is subjective of course )

Is there a workaround for this?

Deathwatch

Does your program require <? extends JavaFileObject> or can it use just <JavaFileObject> or an interface? – Susannahsusanne

I feel your pain - have you looked at Lombok and it's use of val for inferred types? (projectlombok.org/features/val.html) It doesn't clear the whole mess, but it does shrink the code. var in C# provides a similar solution. – Phosphene

An obvious workaround is to use raw types inside your method, if you accept the compiler warnings and the risks of type errors, but I agree that the clean code is ugly. – Wildman

@Susannahsusanne I need <? extends JavaFileObject> I'll use it later – Deathwatch

Answer 1

You define one type parameter named T. Then you can use T within your generic like this:

private <T extends JavaFileObject> Map<String, List<Diagnostic<T>> toMap(List<Diagnostic<T> diagnostics) {
    Map<String, List<Diagnostic<T>> result = new HashMap<String, List<Diagnostic<T>>();
    for (Diagnostic<T> d : diagnostics ) {
        List<Diagnostic<T>> list = null;
        if ( !result.containsKey(d.getCode())) {
            list = new ArrayList<Diagnostic<T>>();
            result.put( d.getCode(), list );
        } else {
            list = result.get( d.getCode() );
        }
        assert list != null;
        list.add( d );
    }
    return result;
}

Above you will see the type parameter defined as <T extends JavaFileObject> and you reuse T everywhere you need to. This will make it a bit cleaner.

Tortoni

If you want to make it even more cleaner, just create a type that encapsulates List<Diagnostic<T>> – Tortoni

better if <D extends Diagnostic> Map<String, List<D>> toMap( List<D> diagnostics ) - the method only cares about that it's a list of some Diagnostic objects, nothing else. – Shrill

Not sure, that method is used by another piece which in turn do cares about the JavaFileObject stuff. I can cast it but... :-/ – Deathwatch

That will not work because Diagnostic doesn't know anything regarding JavaFileObject, how will it know its type? Diagnostic and JavaFileObject are two different entities that are not related with each other. The only relation is that Diagnostic aggregates a JavaFileObject – Tortoni

@irreputable: Use <D extends Diagnostic<?>>, at least. – Rondelet

type inference will know that D=Diagnostic<? extends JavaFileObject> – Shrill

Answer 2

In Scala, this would look something like:

// collections are immutable by default, but we want the mutable flavour
import collection.mutable

// An alias so we don't keep repeating ourself
type DiagMultiMap[T] = mutable.Map[String, mutable.Set[Diagnostic[T]]]

//pimp DiagMultiMap with the addDiagnostic method
class MapDiag[T](theMap: DiagMultiMap[T]) {
  def addDiagnostic(d: Diagnostic[T]): Unit = {
    val set = theMap.getOrElseUpdate(d.getCode) {mutable.Set.empty}
    set += d
  }
}

//an implicit conversion to enable the pimp
implicit def mapDiagPimp[T](theMap: DiagMultiMap[T]) = new MapDiag(theMap)

//This is how we make one
def mkDiagnosticMultiMap[T](entries: Seq[Diagnostic[T]]): DiagMultiMap[T] = {
  val theMap = new mutable.HashMap[String, mutable.Set[Diagnostic[T]]]()
  entries foreach { theMap addDiagnostic _ }
  theMap
}

It's not tested, as I have no access to the code for Diagnostic

---

UPDATE

This'll teach me to post late at night, it's actually far easier...

Given any sequence of Diagnostic objects:

val diags = List(new Diagnostic(...), new Diagnositic(...), ...)

they can easily be grouped with a single method:

val diagMap = diags.groupBy(_.getCode)

---

But it's a little bit more complicated than that!

A bigger problem is that Diagnostic is part of the Java standard library, so you're not able to rewrite it with variance annotations (more on that after the code). A wrapper would do the trick though, and fortunately it's not too big:

class RichDiagnostic[S+](underlying: Diagnostic[S]) {
  def code: String = underlying.getCode
  def columnNumber: Long = underlying.getColumnNumber
  def endPosition: Long = underlying.getEndPosition
  def kind: Diagnostic.Kind = underlying.getKind
  def lineNumber: Long = underlying.getLineNumber
  def messageFor(locale: Locale): String = underlying.getMessage(locale) 
  def position: Long = underlying.getPosition
  def source: S = underlying.getSource
  def startPosition: Long = underlying.getStartPosition
  implicit def toUnderlying: Diagnostic[S] = underlying
}

The + in [S+] marks this class as covariant, so a RichDiagnostic[A] is considered to be a subclass of RichDiagnostic[B] if A is a subclass of B. This is the key to avoiding nasty generic signatures, no more <? extends T> or <? super T>!

It's easy enough to use, too:

val richDiags = diags.map(d => new RichDiagnostic(d))
val diagMap = richDiags.groupBy(_.code)

If the Diagnostics are originally supplied as a Java List then methods like map won't automatically be available to you, but conversion is trivial:

import collection.JavaConverters._

//the toList isn't strictly necessary, but we get a mutable Buffer otherwise
val richDiags = diagsFromJava.asScala.toList.map(d => new RichDiagnostic(d))
val diagMap = richDiags.groupBy(_.code)

Building this collection is a one-shot operation, and will have to be repeated if entries are added to the underlying list, but I suspect that won't be a problem.

Arawak

+1 Thanks for the answer, Diagnostic is available on: javax.tool.Diagnostic in Java1.6 – Deathwatch

Cool, I'll go back and update the answer once I've looked at that class. – Arawak

Answer 3

Great example. In the generic version, there are 19 type arguments; in the raw version, there is only 1 cast. Since this is just a private method, I'd go with the raw version. Even if it's more public, it can still keep the raw method body, but with full generic signature. Probably something like

Map<String, List<Diagnostic<? extends JavaFileObject>>> 
toMap( List<Diagnostic<? extends JavaFileObject>> diagnostics )
{
    Map result = new HashMap();
    for( Diagnostic d  : diagnostics ) 
    {
        List list = (List)result.get( d.getCode() );
        if(list==null)
            result.put( d.getCode(), list=new ArrayList());
         list.add( d );
    }
    return result;
}

With more general typing in signature, and Java 7, we can have

<D extends Diagnostic<?>>
Map<String, List<D>> toMap( List<D> diagnostics )
{
    Map<String, List<D>> result = new HashMap<>();
    for( D d  : diagnostics ) 
    {
        List<D> list = result.get( d.getCode() );
        if(list==null)
            result.put( d.getCode(), list=new ArrayList<>());
         list.add( d );
    }
    return result;
}

void test()
{
    List<Diagnostic<? extends JavaFileObject>> x = null;

    Map<String, List<Diagnostic<? extends JavaFileObject>>> map = toMap(x);
}

8 type arguments.

Shrill

It's worth noting that your second example is still a significant improvement over the code in the question even if (to stay compatible with Java 6) we don't use <>. – Tonsillectomy

Answer 4

Personally I would try breaking something like this (Eclipse compiled - not tried running)

private class MapDiag extends HashMap<String, List<Diagnostic<? extends JavaFileObject>>>{
    private static final long serialVersionUID = 1L;

    void add(Diagnostic<? extends JavaFileObject> d){
      List<Diagnostic<? extends JavaFileObject>> list = null;
      if (containsKey(d.getCode())){
        list = get(d.getCode());
      }
      else {
        list = new ArrayList<Diagnostic<? extends JavaFileObject>>();
        put( d.getCode(), list );
      }
      list.add(d);
    }
  }

  private MapDiag toMap2( List<Diagnostic<? extends JavaFileObject>> diagnostics ) {
    MapDiag result = new MapDiag();
    for ( Diagnostic<? extends JavaFileObject> d : diagnostics ) {
      result.add(d);
    }
    return result;
  }

Thrush

+1 Believe me or not, I was actually in the process to do this: class DiagnosticList extends ArrayList<Diagnostic<? extends JavaFileObject>>{} :) – Deathwatch

Answer 5

I think the 'answer' has been arrived at by some of the comments here but I don't think anyone so far has given the canonical formulation.

private <T extends Diagnostic<? extends JavaFileObject>>
        Map<String, List<T>> toMap(List<T> diagnostics) {
    Map<String, List<T>> result = new HashMap<String, List<T>>();
    for (T d : diagnostics) {
        List<T> list = null;
        if (!result.containsKey(d.getCode())) {
            list = new ArrayList<T>();
            result.put(d.getCode(), list);
        } else {
            list = result.get(d.getCode());
        }
        assert list != null;
        list.add(d);
    }
    return result;
}

The introduction of the type parameter greatly simplifies the internals of the method, while keeping the expressiveness of the signature.

It should be noted that this is a different method to the question as posed, but on balance is probably more correct. The difference being the method given here will ensure that the parameterised type of the Diagnostic is the same for both the input and output of the method.

Unfortunately in this case the invocation of the two constructors prevents us making further use of type parameters (in particular for the Map), though if we were willing to permit ourselves a cast we could make the method even more terse.

Marriott

Answer 6

First, isn't your method wrong?...I mean, shouldn't it be more like

List<T> list = null;
if (!result.containsKey(d.getCode())) {
    list = newArrayList();          
} else {
    list = result.get(d.getCode());
}   
result.put(d.getCode(), list);

Also, you can always emulate the diamond operator with static utility methods that give you some sort of type inference. That is to say

public static <K, V> HashMap<K, V> newHashMap() {
    return new HashMap<K, V>();
}

public static <T> ArrayList<T> newArrayList() {
    return new ArrayList<T>();
}

and then your method will look like

private Map<String, List<Diagnostic<? extends JavaFileObject>>> toMap(List<Diagnostic<? extends JavaFileObject>> diagnostics) {
    Map<String, List<Diagnostic<? extends JavaFileObject>>> result = newHashMap();
    for (Diagnostic<? extends JavaFileObject> d : diagnostics) {
        List<Diagnostic<? extends JavaFileObject>> list = null;
        if (!result.containsKey(d.getCode())) {
            list = newArrayList();
            result.put(d.getCode(), list);
        } else {
            list = result.get(d.getCode());
        }
        assert list != null;
        list.add(d);
    }
    return result;
}

At least instantiations will be smaller.... Note that you may already have this utility methods if you are using google guava library. And if you combine it with the answer Curtain Dog gave you, you get

    private <T extends Diagnostic<? extends JavaFileObject>> Map<String, List<T>> toMap(List<T> diagnostics) {
    Map<String, List<T>> result = newHashMap();
    for (T d : diagnostics) {
        List<T> list = null;
        if (!result.containsKey(d.getCode())) {
            list = newArrayList();
            result.put(d.getCode(), list);
        } else {
            list = result.get(d.getCode());
        }
        assert list != null;
        list.add(d);
    }
    return result;
}

Chatman

Answer 7

Mashing up everyone's here suggestions, this is what I did:

I created a new class DiagnosticList to wrap the ArrayList<Diagnostic<? extends JavaFileObject>>

It is dead simple:

static final class DiagnosticList 
extends ArrayList<Diagnostic<? extends JavaFileObject>>{
    // no arg constructor 
    public DiagnosticList(){}
    // Using a list
    public DiagnosticList(List<Diagnostic<? extends JavaFileObject>> diagnostics){
        super( diagnostics);
    }
}

And then I could cange the method signature.

private Map<String, DiagnosticList> toMap( DiagnosticList diagnostics ) {
    Map<String, DiagnosticList> result = new HashMap<String, DiagnosticList>();
    for ( Diagnostic<? extends JavaFileObject> d : diagnostics ) {
        DiagnosticList list = result.get(d.getCode());
        if( list == null ) {
          result.put( d.getCode(), (list = new DiagnosticList()));
        }
        list.add( d );
    }
    return result;
}

Which is way lot readable.

While I might change the original program semantics, I think I will benefit in maintainability.

Deathwatch