sevntu-checkstyle / sevntu.checkstyle

Additional Checkstyle checks, that could be added as extension to EclipseCS plugin and maven-checkstyle-plugin, Sonar checkstyle plugin, extension for CheckStyle IDEA plugin.
http://sevntu-checkstyle.github.io/sevntu.checkstyle/
191 stars 146 forks source link

New Check: SingleMethodTypeParameter #643

Open romani opened 6 years ago

romani commented 6 years ago

Quote from book "Effective Java", edition 3 , "ITEM 31: USE BOUNDED WILDCARDS TO INCREASE API FLEXIBILITY 145":

There is one more wildcard-related topic that bears discussing. There is a
duality between type parameters and wildcards, and many methods can be
declared using one or the other. For example, here are two possible declarations
for a static method to swap two indexed items in a list. The first uses an
unbounded type parameter (Item 30) and the second an unbounded wildcard:

// Two possible declarations for the swap method
public static <E> void swap(List<E> list, int i, int j);
public static void swap(List<?> list, int i, int j);

Which of these two declarations is preferable, and why? In a public API, the
second is better because it’s simpler. You pass in a list—any list—and the method
swaps the indexed elements. There is no type parameter to worry about. As a rule,
if a type parameter appears only once in a method declaration, replace it with
a wildcard. If it’s an unbounded type parameter, replace it with an unbounded
wildcard; if it’s a bounded type parameter, replace it with a bounded wildcard.

There’s one problem with the second declaration for swap. The straightfor-
ward implementation won’t compile:

public static void swap(List<?> list, int i, int j) {
list.set(i, list.set(j, list.get(i)));
}

Trying to compile it produces this less-than-helpful error message:

Swap.java:5: error: incompatible types: Object cannot be
converted to CAP#1
list.set(i, list.set(j, list.get(i)));
^
where CAP#1 is a fresh type-variable:
CAP#1 extends Object from capture of ?

It doesn’t seem right that we can’t put an element back into the list that we just
took it out of. The problem is that the type of list is List<?>, and you can’t put
any value except null into a List<?>. Fortunately, there is a way to implement
this method without resorting to an unsafe cast or a raw type. The idea is to write a
private helper method to capture the wildcard type. The helper method must be a
generic method in order to capture the type. Here’s how it looks:

public static void swap(List<?> list, int i, int j) {
swapHelper(list, i, j);
}
// Private helper method for wildcard capture
private static <E> void swapHelper(List<E> list, int i, int j) {
list.set(i, list.set(j, list.get(i)));
}

The swapHelper method knows that list is a List<E>. Therefore, it knows
that any value it gets out of this list is of type E and that it’s safe to put any value of
type E into the list. This slightly convoluted implementation of swap compiles
cleanly. It allows us to export the nice wildcard-based declaration, while taking
advantage of the more complex generic method internally. Clients of the swap
method don’t have to confront the more complex swapHelper declaration, but
they do benefit from it. It is worth noting that the helper method has precisely the
signature that we dismissed as too complex for the public method.
In summary, using wildcard types in your APIs, while tricky, makes the APIs
far more flexible. If you write a library that will be widely used, the proper use of
wildcard types should be considered mandatory. Remember the basic rule:
producer-extends, consumer-super (PECS). Also remember that all comparables
and comparators are consumers.

should be applicable only to public methods with demand to make "same" method but as private.

yaziza commented 4 years ago

Hi @romani,

i have started working on this issue and i have two questions:

  1. How should we handle methods with single method parameter containing multiple types, e.g.

abstract <K, V> void clearCache(Map<K,V> cache);

or

abstract <K> void clearCache(Map<K,?> cache);

  1. In sevntu-checks i got some violation in code, which i am not sure how to handle correctly, e.g.

public static Set<String> getPackages(Set<Class<?>> modules) this should not be reported as violation, right ?

or

public static Node findElementByTag(Set<Node> nodes, String tag)

You can check the code, which is almost done. After clarifying this, i will fix the build and update my pull request accordingly.