patternjavaMinor
Collecting data from Vector to Map
Viewed 0 times
mapcollectingfromdatavector
Problem
I wrote the following piece of code and want to hear your opinion, in this snippet I have a vector called
Any comments on the readability, writing manner, other stuff will be appritiated.
Definition:
Code:
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 > aggregator;
protected Vector aggregateFeaturesForML = new Vector(); //Single result has 3 fields: IP, key, valueCode:
String previousIp = aggregateFeaturesForML.get(0).getSourceip();
if(!aggregator.containsKey(previousIp))
{
aggregator.put(previousIp, new HashMap());
}
HashMap histogram = new HashMap();
for(int iterator=0;iterator mapForIP = aggregator.get(previousIp);
mapForIP.put(key, histogram.toString());
aggregator.put(previousIp, mapForIP);
if(!aggregator.containsKey(ip))
{
aggregator.put(ip, new HashMap());
}
histogram.clear();
previousIp = ip;
}
histogram.put(sr.getKey(), sr.getValue());
}
HashMap mapForIP = aggregator.get(previousIp);
mapForIP.put(key, histogram.toString());
aggregator.put(previousIp, mapForIP);Solution
- Beware of hard-coding the first element of a list like
aggregateFeaturesForML.get(0). This can throw an exception isaggregateFeaturesForMLis empty so you should check for that first.
-
Do not compare String with
== (or !=). In the followingif(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 mapForIP = aggregator.get(previousIp);use:
Map 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> aggregator =
aggregateFeaturesForML.stream()
.collect(Collectors.groupingBy(
SingleResult::getSourceip,
Collectors.toMap(SingleResult::getKey, SingleResult::getValue)
));Code Snippets
if(ip != previousIp)if(!ip.equals(previousIp))HashMap<String, String> mapForIP = aggregator.get(previousIp);Map<String, String> mapForIP = aggregator.get(previousIp);Map<String, Map<String, String>> aggregator =
aggregateFeaturesForML.stream()
.collect(Collectors.groupingBy(
SingleResult::getSourceip,
Collectors.toMap(SingleResult::getKey, SingleResult::getValue)
));Context
StackExchange Code Review Q#118609, answer score: 5
Revisions (0)
No revisions yet.