4
\$\begingroup\$

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);
\$\endgroup\$
5
\$\begingroup\$
  • 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)
                          ));
\$\endgroup\$
  • \$\begingroup\$ Hello and thank you for your answer, very important notes! I will take them into account and should try this collector thing! \$\endgroup\$ – Anton.P Feb 2 '16 at 10:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.