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 am looking for clean way to accumulate amounts within a input dataset. In this example, there are 18 rows of data, and the output is a 3 rows of data, accumulated by Key, by ExpenseType.

My biggest concern is whether hashMap is the correct utility for this, or if there is an easier/cleaner way.

FamilyExpenseProcessor.java:

import java.math.BigDecimal;
import java.util.HashMap;
import java.util.Iterator;
import java.util.Map;
import java.util.Map.Entry;

public class FamilyExpenseProcessor {

    enum ExpenseType {
        Grocery, Entertainment,Transportation
    }

    public static void main(String[] args) {

        Map<String, FamilyExpense> feHashMap = new HashMap<>();

        accumulate(feHashMap, "Jeffersons",ExpenseType.Entertainment, new BigDecimal(5.00));
        accumulate(feHashMap, "Jetsons", ExpenseType.Entertainment, new BigDecimal(10.00));
        accumulate(feHashMap, "Johnsons", ExpenseType.Entertainment, new BigDecimal(15.00));

        accumulate(feHashMap, "Jeffersons",ExpenseType.Entertainment, new BigDecimal(20.00));
        accumulate(feHashMap, "Jetsons", ExpenseType.Entertainment, new BigDecimal(25.00));
        accumulate(feHashMap, "Johnsons", ExpenseType.Entertainment, new BigDecimal(30.00));

        accumulate(feHashMap, "Jeffersons",ExpenseType.Grocery, new BigDecimal(35.00));
        accumulate(feHashMap, "Jetsons", ExpenseType.Grocery, new BigDecimal(40.00));
        accumulate(feHashMap, "Johnsons", ExpenseType.Grocery, new BigDecimal(45.00));

        accumulate(feHashMap, "Jeffersons",ExpenseType.Grocery, new BigDecimal(50.00));
        accumulate(feHashMap, "Jetsons", ExpenseType.Grocery, new BigDecimal(55.00));
        accumulate(feHashMap, "Johnsons", ExpenseType.Grocery, new BigDecimal(60.00));

        accumulate(feHashMap, "Jeffersons",ExpenseType.Transportation, new BigDecimal(15.00));
        accumulate(feHashMap, "Jetsons", ExpenseType.Transportation, new BigDecimal(20.00));
        accumulate(feHashMap, "Johnsons", ExpenseType.Transportation, new BigDecimal(25.00));

        accumulate(feHashMap, "Jeffersons",ExpenseType.Transportation, new BigDecimal(30.00));
        accumulate(feHashMap, "Jetsons", ExpenseType.Transportation, new BigDecimal(35.00));
        accumulate(feHashMap, "Johnsons", ExpenseType.Transportation, new BigDecimal(40.00));

        Iterator<Map.Entry<String, FamilyExpense>> entries = feHashMap.entrySet()
                .iterator();

        while (entries.hasNext()) {
            Entry<String, FamilyExpense> entry = entries.next();
            System.out.println("Key = " + entry.getKey() + ", Grocery = "
                    + entry.getValue().getGroceryExpense());
            System.out.println("Key = " + entry.getKey() + ", Entertainment = "
                    + entry.getValue().getEntertainmentExpense());
            System.out.println("Key = " + entry.getKey() + ", Transportation = "
                    + entry.getValue().getTransportationExpense());
        }

    }

    private static void accumulate(Map<String, FamilyExpense> feMap,
            String key, ExpenseType expenseType, BigDecimal value) {

        switch (expenseType) {
        case Grocery:

            if (feMap.containsKey(key)) {
                feMap.get(key).setGroceryExpense(
                        feMap.get(key).getGroceryExpense().add(value));
            } else {

                FamilyExpense fe = new FamilyExpense();
                fe.setGroceryExpense(value);
                feMap.put(key, fe);
            }
            break;
        case Entertainment:

            if (feMap.containsKey(key)) {
                feMap.get(key).setEntertainmentExpense(
                        feMap.get(key).getEntertainmentExpense().add(value));
            } else {

                FamilyExpense fe = new FamilyExpense();
                fe.setEntertainmentExpense(value);
                feMap.put(key, fe);
            }
            break;
        case Transportation:

            if (feMap.containsKey(key)) {
                feMap.get(key).setTransportationExpense(
                        feMap.get(key).getTransportationExpense().add(value));
            } else {

                FamilyExpense fe = new FamilyExpense();
                fe.setTransportationExpense(value);
                feMap.put(key, fe);
            }
            break;
        }
    }
}

FamilyExpense.java:

 import java.math.BigDecimal;

 class FamilyExpense {

    private String familyId;
    private BigDecimal groceryExpense;
    private BigDecimal entertainmentExpense;
    private BigDecimal transportationExpense;

    public FamilyExpense(){
        this.groceryExpense = new BigDecimal(0);
        this.transportationExpense = new BigDecimal(0);
        this.entertainmentExpense = new BigDecimal(0);

    }
    public String getFamilyId() {
        return familyId;
    }

    public void setFamilyId(String familyId) {
        this.familyId = familyId;
    }

    public BigDecimal getGroceryExpense() {


        return groceryExpense;
    }

    public void setGroceryExpense(BigDecimal groceryExpense) {
        this.groceryExpense = groceryExpense;
    }

    public BigDecimal getEntertainmentExpense() {

        return this.entertainmentExpense;
    }

    public void setEntertainmentExpense(BigDecimal entertainmentExpense) {
        this.entertainmentExpense = entertainmentExpense;
    }

    public BigDecimal getTransportationExpense() {
        return transportationExpense;
    }

    public void setTransportationExpense(BigDecimal transportationExpense) {
        this.transportationExpense = transportationExpense;
    }
}
share|improve this question

2 Answers 2

up vote 2 down vote accepted

Well I have changed your code a bit to make it cleaner.It is recommended to have a uniform interface for methods, so now you have two methods in FamilyExpense class is addExpense and another is getExpense rather explicitly calling each method.

I am not sure why you have used getFamilyId and setFamilyId.

public static void main(String[] args) {

    Map<String, FamilyExpense> feHashMap = new HashMap<String, FamilyExpense>();
    feHashMap.put("Jeffersons", new FamilyExpense());
    feHashMap.put("Jetsons", new FamilyExpense());
    feHashMap.put("Johnsons", new FamilyExpense());

    accumulate(feHashMap, "Jeffersons", ExpenseType.Entertainment, new BigDecimal(5.00));
    accumulate(feHashMap, "Jetsons", ExpenseType.Entertainment, new BigDecimal(10.00));
    accumulate(feHashMap, "Johnsons", ExpenseType.Entertainment, new BigDecimal(10.00));


    Iterator<Map.Entry<String, FamilyExpense>> entries = feHashMap.entrySet()
            .iterator();

    while (entries.hasNext()) {
        Entry<String, FamilyExpense> entry = entries.next();
        System.out.println("Key = " + entry.getKey() + ", Grocery = "
                + entry.getValue().getExpense(ExpenseType.Grocery));
        System.out.println("Key = " + entry.getKey() + ", Entertainment = "
                + entry.getValue().getExpense(ExpenseType.Entertainment));
        System.out.println("Key = " + entry.getKey() + ", Transportation = "
                + entry.getValue().getExpense(ExpenseType.Transportation));
    }

}

 public static void accumulate(Map<String, FamilyExpense> feMap,
                              String key, ExpenseType expenseType, BigDecimal value) {

    FamilyExpense familyExpense=null;

    if(feMap.containsKey(key)){
        familyExpense=feMap.get(key);
        familyExpense.addExpense(value,expenseType);
    }
    else{
        FamilyExpense newFamily = new FamilyExpense();
        newFamily.addExpense(value,expenseType);
        feMap.put(key,newFamily);
    }
}

This is your family expense class.

import java.math.BigDecimal;

public class FamilyExpense {

private String familyId;
private BigDecimal groceryExpense;
private BigDecimal entertainmentExpense;
private BigDecimal transportationExpense;

public FamilyExpense(){
    this.groceryExpense = new BigDecimal(0);
    this.transportationExpense = new BigDecimal(0);
    this.entertainmentExpense = new BigDecimal(0);
}

public BigDecimal getExpense(FamilyExpenseProcessor.ExpenseType expenseType){
    switch (expenseType){
        case Entertainment:
            return this.entertainmentExpense;
        case Grocery:
            return this.groceryExpense;
        case Transportation:
            return this.transportationExpense;
        default:
            return new BigDecimal(0.0);
    }
}

public void addExpense(BigDecimal expense, FamilyExpenseProcessor.ExpenseType expenseType){
    switch (expenseType){
        case Entertainment:
            this.entertainmentExpense=this.entertainmentExpense.add(expense);
            break;
        case Grocery:
            this.groceryExpense=groceryExpense.add(expense);
            break;
        case Transportation:
            this.transportationExpense=this.transportationExpense.add(expense);
            break;
    }
   }
}
share|improve this answer

I would suggest adding a method addExpense(ExpenseType expenseType, BigDecimal amount) to FamilyExpense. That way your accumulate method could look like this:

private static void accumulate(Map<String, FamilyExpense> expenses, String key, ExpenseType expenseType, BigDecimal amount) {
  if (expenses.containsKey(key)) {
    expenses.get(key).addExpense(expenseType, amount);
    return;
  }

  FamilyExpense expense =  new FamilyExpense();
  expense.addExpense(expenseType, amount);
  expenses.put(key, expense);
}

You can also iterate through the entries using entrySet:

for (Map.Entry<String, FamilyExpense> entry : entries.entrySet()) {
  ...
}
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.