Archive for February, 2013

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++) {		
    errorItems.add(errorItemsFromNode[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 = iterator.next();			
      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>();
  values.add("1");
  values.add("2");
 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 {
		getReasonsid();
	}
	
	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(CastTest.java:18)
	at CastTest.testSample(CastTest.java:11)

Correctness – Invocation of toString on an array

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

String[] vars = { "1", "2" };
System.out.println(vars.toString());

the output produced isn’t really helpfull

[Ljava.lang.String;@10b4199

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

String[] vars = { "1", "2" };
if (LOG.isDebugEnabled()) {
    LOG.debug(ArrayUtils.toString(vars));
}
[1, 2]
Advertisements

4 Comments