S4738: ok, but not for com.google.common.base.Joiner.on

  • SonarQube 7.9, maven, jdk 8, Java

I agree with most of the rule, except for the part about Joiner. This should be either removed from the rule or the rule should be adapted to investigate how Joiner is used.

Some examples:

final Map<String, String> map = ImmutableMap.<String, String>builder()
    .put("key1", "value1")
    .put("key2", "value2")
    .build();

final String guava = Joiner.on("\n\t").withKeyValueSeparator(":").join(map);
final String java = map.entrySet().stream().map(e -> e.getKey() + ":"  + e.getValue()).collect(Collectors.joining("\n\t"));

=> guava is in my opinion a lot more readable than java

final Collection<String> collection = Arrays.asList("a", "b", "c");
final StringBuilder buf = new StringBuilder();
buf.append("x").append(collection.size()).append("y");
Joiner.on("\n\t").appendTo(buf, collection);
final String guava = buf.toString();

I’m not sure how you can use the java join api to add to an existing buffer. Note that above code is kept small. In reality a lot more interaction is done with the buffer.

If the rule would be configurable to allow to select which api to trigger on, that would be perfect.

Hello @sbv,

One of the point of the rule is to:

[…] ease maintenance: developers don’t need to learn how to use two APIs and can stick to the standard one.

For the first example, guava is more concise, but I would argue that the java version is not that bad, it’s clear to me what is happening, and not a lot less readable, especially if you are not familiar with guava.

For the second example, why not something like this?

buf.append(String.join("\n\t", collection));

That being said… I understand your concern, and in fact, I am also divided. What makes me doubt is that Joiner is the only one in the list that guava don’t explicitly advice to avoid…

I propose to let it as it is for now, but I’m eager to discuss it further, if it happens that Joiner is a source of noise, removing it from the list is definitely something to consider.

Hi @Quentin,

Thanks for your reply. For the first one, I agree it’s not that bad, but (in my opinion) Guava is better. Also in the Java version a lot of useless strings will be created to combine key and value. In the Guava version there’s a single buffer being filled.

The second example has the same issue. It will create a needless String instead of directly adding to the buffer. Sure, in above examples it doesn’t really matter. These are simplified examples though. In reality a large buffer is created with lots of data and the collections/maps contain a lot more data.

It’s a good rule, and I’d like to keep it for the other warnings, but as said, there’s no Java API yet to my knowledge that is buffer friendly. If the rule would be configurable to select which subrules to check, that could work for everyone.

As it is now, we need to use @SuppressWarnings for these cases.

1 Like

Okay, it makes sense, the fact that there is no alternative providing the same feature convinced me. I created a ticket to remove joiner from the list.

I’m not convinced by the idea to configure the rule though, my point of vue is that the list should not be arguable and therefore not configurable.
In addition, if you still want to exclude an additional specific guava class, you can use S3688.

Hope it makes sense.

Best,
Quentin

1 Like

Good news, that’ll make it better for our developers! Thanks @Quentin!