Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I am calculating the number of people within a given age range. This is my current approach.

Note:

  • The persons is a List<Person>
  • The getDiffYears() returns the number of years between now and DOB of the person. It includes the month and day as well.
  • The method will be returning a HashMap<String, Integer>() (Can be changed if there is a better alternative.)

The code has been tested and is working. I wanted to know if there is a more efficient approach without using this many if statements?

for (Person p: persons) {
 Date dob = p.getDob();
 if (dob != null) {
  years = getDiffYears(p.getDob());
  if (years < 10) {
   a++;
  } else if (years < 20) {
   b++;
  } else if (years < 30) {
   c++;
  } else if (years < 40) {
   d++;
  } else if (years < 50) {
   e++;
  } else if (years < 60) {
   f++;
  } else if (years < 70) {
   g++;
  } else if (years < 80) {
   h++;
  } else if (years < 90) {
   i++;
  } else if (years < 100) {
   j++;
  } else {
   k++;
  }
 } else {
  k++;
 }
}
map.put("00 - 09", a);
map.put("10 - 19", a);
map.put("20 - 29", b);
map.put("30 - 39", c);
map.put("40 - 49", e);
map.put("50 - 59", f);
map.put("60 - 69", g);
map.put("70 - 79", h);
map.put("80 - 89", i);
map.put("90 - 99", j);
map.put("Unknown", k);
share|improve this question
    
Welcome to CR! Are you on Java 8? – h.j.k. Jan 8 at 15:15
    
I would argue that being over 100 is not unknown just in the category 100+. Since you're using range maybe 100-infinity (I know it's useful for you probably, I'm just pointing it out) – Marc-Andre Jan 8 at 15:19
2  
@h.j.k. Yes, I am on Java 8. – miqdadamirali Jan 8 at 15:30
    
@Marc-Andre I have them in the same variable as the count won't be used for anything. It is there to simply know how many people's age were not used. – miqdadamirali Jan 8 at 15:31
    
@miqdadamirali I do understand this but I just wanted to point out that classifying people over 100 in a category Unknown is not accurate since you do know their age and in which "range" they are. I suspected you did not use that "range" and why you put it in an unused category. – Marc-Andre Jan 8 at 15:42
up vote 7 down vote accepted

First of all you have a typo with your values. You accidentally have a twice when putting to your map, and then you end up missing d:

map.put("00 - 09", a);
map.put("10 - 19", a);
map.put("20 - 29", b);
map.put("30 - 39", c);
map.put("40 - 49", e);

These single letter variables aren't helping you. Instead have an int array that you increment. You could then just increment by index, and since you're using a simple numerical set up, you could just use years/10 as your index.

Something like this:

int counts[] = new int[11];
if (years == null or years > 100){
    counts[10]++;
} else {
    counts[years/10]++;
}

ints are rounded down so if years = 19 then you get 19/10, which is 1.

You can then use these values for your mapping:

map.put("00 - 09", counts[0]);
map.put("10 - 19", counts[1]);
map.put("20 - 29", counts[2]);

I'm sure there's a way to loop over those or make the mapping more directly, but I'm not personally familiar with it.

share|improve this answer
    
Thank you for pointing out the mistake :). 1. The issue I run into when using the array is that I do not have the option of also storing the range the count corresponds to. 2. A number such as 15, when divided by 10. Would the result be rounded up or down? because you can't have value at index position 1.5. – miqdadamirali Jan 8 at 15:29
2  
@miqdadamirali integers are rounded down, so even 19/10 becomes 1. Also I wasn't suggesting you just keep the array, I intended for you to then use putat the end with the array values. I'll amend both these notes. – SuperBiasedMan Jan 8 at 15:31
    
You made a good point. I'll wait out a day and see if someone else responds. Thanks – miqdadamirali Jan 8 at 15:38
2  
Aren't the indices 0-10 (so 11 elements?) Wouldn't [11] overflow? – Benjamin Gruenbaum Jan 8 at 16:07
    
@BenjaminGruenbaum You are correct, I rounded it right at first then did it wrong when figuring out what index "Unknown" should be. – SuperBiasedMan Jan 8 at 16:12

Java 8 streams

Aside from the typos pointed out by @SuperBiasedMan, you can use Java 8's stream-based processing as an alternative for generating your Map result.

private static final int UPPER_LIMIT = 100;
private static final String[] KEYS = new String[]{ "00 - 09", /* ... */ "Unknown" };

private static int categorize(int year) {
    return Math.min(year, UPPER_LIMIT) / 10;
}

Map<String, Long> results = persons.stream()
                    .map(Person::getDob)
                    .filter(Objects::nonNull)
                    .map(ThisClass::getDiffYears)
                    .map(ThisClass::categorize)
                    .collect(Collectors.groupingBy(ageCategory -> KEYS[ageCategory],
                                Collectors.counting()));
  1. Define the UPPER_LIMIT of your age categorization and the desired category names.
  2. From a Stream of your Person objects, map() to their birthdays by calling Person::getDob as method reference.
  3. filter() null birthdays by using Objects::nonNull.
  4. map() birthdays to ages by calling your getDiffYears(Date) method. Note that if it is not a static method, the method reference then becomes this::getDiffYears.
  5. map() ages to the age category by calling categorize(int) to get the desired array indices for KEYS. If you have not realized yet, ThisClass is just a placeholder for the actual class name.
  6. collect() the age categories into the desired Map result groupingBy() our categorization and counting() the elements per category.

Other advice

  • Your method should return a Map instead of HashMap so that callers of this method only need to deal with the interface, and not the implementation.
  • You may want to consider how to handle for null inputs to getDiffYear(Date). Should it return a negative value?
  • You can also experiment with deriving the category names in a programmatic manner, instead of using literal Strings.
  • You can even think about making your age categorization depend on a configurable upper limit (100) and band (10). For example, how easy will it be to treat the upper limit as 101 (UPPER_LIMIT = 101) and a band of 11 years per category (hint: take a closer look at the suggested code...)?
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.