Through the eyes of sonar : Collections.

Collections usage

After Exception handling, Immutable objects, Complexity, Naming and comments, let’s see what sonar can teach you about Collections.

Use Arrays As List

Use asList instead of tight loops

 final SWTBotTreeItem[] errorItemsFromNode =...
 for (int j = 0; j < errorItemsFromNode.length; j++) {		

Can be rewritten as :

 errorItems = Arrays.asList(errorItemsFromNode)

Note that the returned list is a non modifiable list. So if another part of the code tries to modify it will receive a UnsupportedOperationException

 java.util.List errorItems = new java.util.ArraysList(Arrays.asList(errorItemsFromNode));

Performance – Inefficient use of keySet iterator instead of entrySet iterator

The following code requires to do lookup (files.get(…)) and so evaluating hashCode on the key and while iterating on a Map and the doing a lookup the map implementation

 Map<String, Map<DateTime, File>> files = ...

  for (Iterator<String> iterator = files.keySet().iterator(); iterator.hasNext();) {
      final String serverName =;			
      final Map<DateTime, File> serverStats = files.get(serverName);

You can prevent this, let’s use the entrySet() that will give you the key and associated value in a Map.Entry

Map<String, Map<DateTime, File>> files = ...
  for (Map.Entry<String, List<MailServerStats>> entry : files.entrySet()) {
      final String serverName = entry.getKey();			
      final Map<DateTime, File> serverStats = entry.getValue();

Loose coupling

public class AST
       private HashMap<String, Object> properties = new HashMap<String, Object>();
       public HashMap<String, Object> getProperties() { 
              return properties

Please use collection interfaces (Map, Set, List) instead of concrete implementation (HashMap, HashSet, ArrayList, LinkedList,…) :

 public class AST
       private Map<String, Object> properties = new HashMap<String, Object>();
       public Map<String, Object> getProperties() { 
              return properties

This would allow you to easily switch implementation, in this case may be you want the map to be sorted.
You just need to change the new HashMap… by a new TreeMap.

Replace Vector With List

Consider replacing this Vector with the newer java.util.List
Vector synchronizes on each individual operation. That’s almost never what you want to do.
Note that Vector is synchronized… and ArrayList/LinkedList are not.
So if the synchronized nature is required in your code base
you should use Collections.synchronized

 List list = Collections.synchronizedList(new ArrayList());

Replace Hashtable With Map

Consider replacing this Hashtable with the newer java.util.Map
Hashtable synchronizes on each individual operation. That’s almost never what you want to do.

note that Hashtable is synchronized… and HashMap/TreeMap/… are not.
So if the synchronized nature is required in your code base
you should use Collections.synchronized

 Map list = Collections.synchronizedMap(new HashMap());

Class Cast Exception With To Array

This usage of the Collection.toArray() method will throw a ClassCastException.

  return (String[]) values.toArray();

the signature of the method : Object[] toArray()
the ‘java.lang.ClassCastException: [Ljava.lang.Object; incompatible with [Ljava.lang.String;'
but you have the other method T[] toArray(T[] a)

  return (String[]) values.toArray(new String[values.size()]);

Note that if you use generics you can drop the cast :

  List<String> values = new ArrayList<String>();
 return values.toArray(new String[values.size()]);

Correctness - Impossible downcast of toArray() result

Same explanation as previous rules.

	private static Map<String, String> linksreasonidtogroupid = new HashMap<String, String>();
	public void testSample() throws Exception {
	public static String[] getReasonsid() throws Exception {
		Collection<String> values = linksreasonidtogroupid.values();
		return (String[]) values.toArray();


You endup with :

java.lang.ClassCastException: [Ljava.lang.Object;
	at CastTest.getReasonsid(
	at CastTest.testSample(

Correctness - Invocation of toString on an array

Suppose you want to logs the variables in the following program :

String[] vars = { "1", "2" };

the output produced isn't really helpfull


instead use conditionnal logging and org.apache.commons.lang.ArrayUtils.toString (nullsafe and performant)

String[] vars = { "1", "2" };
if (LOG.isDebugEnabled()) {
[1, 2]
About these ads
  1. #1 by Anon on February 27, 2013 - 7:23 pm

    This could be a post about FindBugs, what else is used to generate such a report ? PMD ?

    • #2 by mestachs on February 27, 2013 - 7:43 pm

      In fact sonar (open source quality platform) combines checkstyle, pmd, findbugs and and their own engine SSLR to produce such report. I think you are right most of the rules comes from findbugs but the goal of my articles is to show “how to fix these violations” and “not blindly” thanks to a small recap of the java theory around each topics.

      But the origins of the detection I don’t really care because sonar unify them. I don’t even need to install these tools in eclipse, I’m using their sonar eclipse plugin and all the developpers in the company will use the same rules as my central sonar instance.

  1. Through the eyes of sonar : Collections. | Software Quality - Sonar by SonarSource |
  2. Through the eyes of sonar: recap. | Don't Make the Same Mistake Twice

Leave a Reply

Fill in your details below or click an icon to log in: Logo

You are commenting using your account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s


Get every new post delivered to your Inbox.

%d bloggers like this: