Collecting data from Vector to Map

Question

I wrote the following piece of code and want to hear your opinion, in this snippet I have a vector called aggregateFeaturesForML which has elements of Class with 3 fields: sourceip, key, value. what I want is to collect all the key-value pairs that have the same sourceip and form a nice histogram (for each IP the keys are unique), for that purpose I collect the key-value pairs in a map called histogram and then use its toString function to print a {key=value, key=value} formation. I store the histograms inside another map called aggregator.

Any comments on the readability, writing manner, other stuff will be appritiated.

Definition:

private HashMap<String, HashMap<String, String> > aggregator;
protected Vector<SingleResult> aggregateFeaturesForML = new Vector<SingleResult>(); //Single result has 3 fields: IP, key, value

Code:

 String previousIp = aggregateFeaturesForML.get(0).getSourceip();
 if(!aggregator.containsKey(previousIp))
 {
 aggregator.put(previousIp, new HashMap<String, String>());
 }
 HashMap<String, String> histogram = new HashMap<String, String>(); 
 for(int iterator=0;iterator<aggregateFeaturesForML.size();iterator++)
 {
 SingleResult sr = aggregateFeaturesForML.get(iterator);
 String ip = sr.getSourceip();
 if(ip != previousIp)
 {
 HashMap<String, String> mapForIP = aggregator.get(previousIp); 
 mapForIP.put(key, histogram.toString());
 aggregator.put(previousIp, mapForIP);
 if(!aggregator.containsKey(ip))
 {
 aggregator.put(ip, new HashMap<String, String>());
 }
 histogram.clear();
 previousIp = ip;
 }
 histogram.put(sr.getKey(), sr.getValue());
 }
 HashMap<String, String> mapForIP = aggregator.get(previousIp);
 mapForIP.put(key, histogram.toString());
 aggregator.put(previousIp, mapForIP);

Answer 1

• Beware of hard-coding the first element of a list like aggregateFeaturesForML.get(0). This can throw an exception if aggregateFeaturesForML is empty so you should check for that first.

• Do not compare String with == (or !=). In the following

   if(ip != previousIp)
  

you are comparing the Strings ip and previousIp with !=. Strings are compared using their equals method, so you should have instead:

 if(!ip.equals(previousIp))

• Prefer to program against interfaces. Instead of

   HashMap<String, String> mapForIP = aggregator.get(previousIp);
  

use:

 Map<String, String> mapForIP = aggregator.get(previousIp);

---

As a side-note, if you are using Java 8, your code could be written a lot more simply using the Stream API, that directly provides a way to group elements with a classifier, using the groupingBy collector. In this case, when two elements are classified the same way, they are collected into a Map with the toMap collector.

Map<String, Map<String, String>> aggregator =
 aggregateFeaturesForML.stream()
 .collect(Collectors.groupingBy(
 SingleResult::getSourceip,
 Collectors.toMap(SingleResult::getKey, SingleResult::getValue)
 ));

• radarbob: The "program against interfaces" reference has a key comment: It doesn't mean or even imply use the interface keyword at all. And HashMap implements Map - it is a Map already but a Map is not a HashMap.