Aggregation on List - Java

Question

Im using the following technologies: Google App Engine, GAE Datastore, Struts2. Common lang java library were also used.

Because of the limitation on aggregation for datastore, im using jagg for aggregation on List of Profile for some sort of report.

Profile object have basic properties like email, name, gender, country, dateCreated, etc.

For Chart Report I have below:

public class ChartReport {

private Date date = null;
private String country = null;
private Map<String, Integer> count = null;      //by gender

public ChartReport() {
    Map<String, Integer> map = new HashMap<String, Integer>();
    map.put("m", 0);
    map.put("f", 0);
    map.put(null, 0);
    count = map;
}

public Date getDate() {
    return date;
}
public void setDate(Date date) {
    this.date = date;
}

public String getCountry() {
    return country;
}
public void setCountry(String country) {
    this.country = country;
}

public Map<String, Integer> getCount() {
    return count;
}
public void setCount(Map<String, Integer> count) {
    this.count = count;
}

}

I used map for count so that report will have different count for male, female and null.

For my Struts2 action, I have below:

    if ( dateFrom != null && dateTo != null )
        list = access.getNewProfiles(dateFrom, dateTo);

    //remove timestamp
    if ( type.equals("Daily")){
        for(Profile p: list)
            p.setDateCreated( DateUtils.truncate(p.getDateCreated(), Calendar.DATE) );
    }else{
        for(Profile p: list)
            p.setDateCreated( DateUtils.truncate(p.getDateCreated(), Calendar.MONTH) );
    }

    List<String> properties = new ArrayList<String>();
    properties.add("dateCreated");
    properties.add("gender");

    List<Aggregator> aggregators = new ArrayList<Aggregator>();
    aggregators.add(new CountAggregator("*"));

    List<AggregateValue<Profile>> aggValues = Aggregations.groupBy(
        list, properties, aggregators);

    Profile profile = null;
    ChartReport report = null;
    reportList = new ArrayList<ChartReport>();
    Date date = null;

    for (AggregateValue<Profile> aggValue : aggValues ){
        profile = aggValue.getObject();

        Aggregator aggregator = aggregators.get(0);     //only one aggregator
        if ( date == null || ! date.equals( profile.getDateCreated() ) ){
            report = new ChartReport();
            report.setDate( profile.getDateCreated() );
            report.getCount().put(profile.getGender(), Integer.parseInt(String.valueOf(aggValue.getAggregateValue(aggregator))));
            reportList.add(report);
        }else{
            report.getCount().put(profile.getGender(), Integer.parseInt(String.valueOf(aggValue.getAggregateValue(aggregator))));
            date = profile.getDateCreated();
            continue;
        }

        date = profile.getDateCreated();
    }

The condition ( date == null || ! date.equals( profile.getDateCreated() ) ) was used for grouping purposes. The same dateCreated value will be in one group.

Everything works perfectly. But I need suggestions, comments, and feedback for my code. Can I improve it's performance in terms of speed? Include in your comment the way I write code.

Thanks a lot.

Answer 1

Can I improve it's performance in terms of speed?

Yes.

As there is no specific value in my one word answer, the same does apply to your question for me. Are there any requirements for speed? If no, then just do not take care about it and save your time. If yes, have your profiled it against the requirements?

Some small things:

private Map<String, Integer> count = null;      //by gender

Well, instead of the comment, you could write:

private Map<String, Integer> mapGenderToCount = null;

and save the comment, prevent everyone from reading it all the time and save it from updates (which noone will take care of)

---

map.put(null, 0);

This was unexpected. What kind of biological or theoretical gender is null?

---

Map<String, Integer> map = new HashMap<String, Integer>();
map.put("m", 0);
map.put("f", 0);
map.put(null, 0);
count = map;

You could directly write:

Map<String, Integer> count = new HashMap<String, Integer>();
count.put("m", 0);
count.put("f", 0);
count.put(null, 0);

There is no difference in your case.

---

public void setCount(Map<String, Integer> count) {
    this.count = count;
}

Is there a specific need for this method? Otherwise I would remove it. I would not provide methods to change my internal data structure. I would avoid the getCount, too.
Instead of

public Map<String, Integer> getCount() {
    return count;
}

You could provide a method

public boolean increaseCount(String gender, int count) {
    ...;
    return if successful (only if needed, otherwise void)
}

or

public boolean increaseFemale(int count) {
    ...;
    return if successful (only if needed, otherwise void)
}
+male

---

You could replace the String-Key for the HasMap by enum Gender:

public enum Gender {
    MALE, FEMALE, UNKNOWN;
}

Map<Gender, Integer> mapGenderToCount

---

if ( type.equals("Daily")){

Looks like an use case for enum, too.Then you could remove the if/else and write something like:

for(Profile p: list)
    p.setDateCreated( DateUtils.truncate(p.getDateCreated(), date.getUnit()) );

---

More a personal preference. Instead of:

    final List<String> properties = new ArrayList<>();
    properties.add("dateCreated");
    properties.add("gender");

You could write:

    final List<String> properties = Arrays.asList("dateCreated", "gender");
    //or if you need mutability
    final List<String> properties = new ArrayList<>(Arrays.asList("dateCreated", "gender"));

---

Profile profile = null;

If you do not need variables outside a loop, do not declare them outside of your loop. Some people will argue with a speed argument to declare them outside, but as I said, just prefer readability over premature optimization.

---

Aggregator aggregator = aggregators.get(0);     //only one aggregator

Then remove the list?

---

This:

    if ( date == null || ! date.equals( profile.getDateCreated() ) ){
        report = new ChartReport();
        report.setDate( profile.getDateCreated() );
        report.getCount().put(profile.getGender(), Integer.parseInt(String.valueOf(aggValue.getAggregateValue(aggregator))));
        reportList.add(report);
    }else{
        report.getCount().put(profile.getGender(), Integer.parseInt(String.valueOf(aggValue.getAggregateValue(aggregator))));
        date = profile.getDateCreated();
        continue;
    }

    date = profile.getDateCreated();

could be simplified to:

    if ( date == null || ! date.equals( profile.getDateCreated() ) ){
        report = new ChartReport();
        report.setDate( profile.getDateCreated() );
        reportList.add(report);
    }
    report.getCount().put(profile.getGender(), Integer.parseInt(String.valueOf(aggValue.getAggregateValue(aggregator))));
    date = profile.getDateCreated();

---

In the end, you could come up with something like this:

if ( dateFrom != null && dateTo != null )
    list = access.getNewProfiles(dateFrom, dateTo);

//remove timestamp
p.setDateCreated( DateUtils.truncate(p.getDateCreated(), type.getUnit()) );

Aggregator countAggregator = new CountAggregator("*");
List<AggregateValue<Profile>> listAggregateValues = Aggregations.groupBy(
            list,
            Arrays.asList("dateCreated", "gender"),
            Arrays.asList(countAggregator)
        );

reportList = new ArrayList<ChartReport>();
Date date = null;
ChartReport report = null;

for (AggregateValue<Profile> listAggregateValuesItem :  listAggregateValues){ // perhaps extract loop to own method with good name
    Profile profile = listAggregateValuesItem.getObject();

    if ( date == null || ! date.equals( profile.getDateCreated() ) ){
        report = new ChartReport();
        report.setDate( profile.getDateCreated() );
        reportList.add(report);
    }

    report.increaseCount(profile.getGender(), Integer.parseInt(listAggregateValuesItem.getAggregateValue(countAggregator).toString()));
    date = profile.getDateCreated();
}

---

Other than that, I do not like to use too much external libraries. In particular fancy ones. They make code hard to read for other people and you need some trust in what is happening. But this is a strongly personal and subject meaning.

Answer 2

The rules of optimization may be helpful here:

1. Don't do it.
2. Don't do it, yet.
3. Profile before optimizing.

We should forget about small efficiencies, say about 97% of the time: premature optimization is the root of all evil. - Donald Knuth (who attributed the observation to C.A.R Hoare)

Above from http://c2.com/cgi/wiki?RulesOfOptimization (the original wiki).

Also, I always wrap the contents of all conditionals and loops in {}'s. This increases readability and reduces the chance of adding another line and mistaking that it is being executed inside of a loop or conditional.