Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm 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, I'm 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 this:

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 this:

    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 its performance in terms of speed?

share|improve this question
    
You may want to look into JodaTime, which is considered a superior implementation than the standard Java handling of date/time. –  Clockwork-Muse Jan 29 '13 at 21:42
    
... It just occurred to me to ask - where is this information being stored? If you have a backend db, that will almost certainly be the fastest way to do this. –  Clockwork-Muse Jan 30 '13 at 19:52
    
it's stored in AppEngine datastore w/c don't allow aggregation query like count, avg, sum, etc. –  JR Galia Jan 30 '13 at 21:50

2 Answers 2

up vote 2 down vote accepted

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.

share|improve this answer
    
I'm assuming a null gender is 'unspecified' - ie it's part of a web form, and some users just won't give that information. –  Clockwork-Muse Jan 30 '13 at 19:50

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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