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 is 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)
                          ));