Calculating multiple car prices from a file - follow-up

Question

My application needs to read a file, which includes a list of cars from different factories, and calculate the prices (by this I mean mean add up their prices).

I suppose my solution is efficient. Is there any better approach to it?

 String factories = "Benz,10000,BMW,3000,Benz,5,Toyota,500,BMW,50000,RR,100000,BMW,100000,Jaguar,100000,Benz,4,Next,100,Benz,3,Benz,2,Benz,1,Toyota,100000,Polo,10,Gall,1000";
 String[] fac = factories.split(",");
 List<Company> trn = new ArrayList();
 for(int i=0;i<fac.length-1;i=i+2){
 trn.add(new Company(fac[i],fac[i+1]));
 }
 Map<String, BigDecimal> map = new HashMap();
 BigDecimal counter = new BigDecimal("0");
 BigDecimal target = new BigDecimal("10000");
 for (Company c : trn) {
 String name = c.getName();
 if (!map.containsKey(name)) {
 counter = new BigDecimal("0");
 for (Company com : trn) {
 if (com.getName().equalsIgnoreCase(name)) {
 counter = counter.add(com.getPrice());
 if ((counter.compareTo(target)) == 1) {
 map.put(name, counter);
 }
 }
 }
 }
 }
 Set<String> set = map.keySet();
 System.err.println(">>" + set.toString());

Output

[RR, Toyota, BMW, Jaguar, Benz]

Answer 1

List<Company> trn = new ArrayList();

You're missing the <> (or <Company> if you're using JDK6 or below) on the right, which means the right side is a raw type and not a generic. Your IDE should be configured to warn you about this.

In addition, what does trn mean? It's completely unclear even from context. You should almost never use abbreviations unless they're industry-standard, and maybe not even then.

Map<String, BigDecimal> map = new HashMap();

This is also a raw type. Its name also tells me nothing. What are you mapping? A better name would be mapTotalPriceByFactoryName, or just totalPriceByFactoryName.

BigDecimal counter = new BigDecimal("0");

BigDecimal has a constant for this, BigDecimal.ZERO. Use it here and below.

for (Company c : trn) {

Another unnecessary abbreviation - call the loop variable company.

counter = new BigDecimal("0");

Is this actually a counter? It seems like a summation, and should be called something like totalPrice accordingly.

if (com.getName().equalsIgnoreCase(name)) {
 counter = counter.add(com.getPrice());
 if ((counter.compareTo(target)) == 1) {
 map.put(name, counter);
 }
}

This code is unclear to me. Why equalsIgnoreCase instead of just equals - do we expect the input file to be inconsistent with itself? That seems like an important caveat to call out in comments.

The behavior of the counter is also very unclear. You only want to store factories with a total price of at least 10000? It would be simpler to do the summation in one step, then filter out the unwanted factories in a subsequent step.

Set<String> set = map.keySet();
System.err.println(">>" + set.toString());

Here you never refer back to the values stored in the map. If you're not going to refer back to them, you might as well just use a List.

As an example, here's how I would store the prices:

Map<String, BigDecimal> map = new HashMap<>();
for (Company company : companies) {
 String name = company.getName();
 if (!map.containsKey(name)) {
 map.put(name, BigDecimal.ZERO);
 }
 BigDecimal totalPrice = map.get(name).add(company.getPrice());
 map.put(name, totalPrice);
}

This becomes an O(N) algorithm instead of the O(N^2) algorithm you're using currently. You can then post-process the map to filter out the companies you aren't interested in.

• Jack: Thanks, what do you mean by doing the summation in one step? the output of map.keySet() is Set<String> I cant change the type of set to List
• Chris Hayes: @Jack See the bottom of my answer for how I would do the summation as its own step using a Map. If you still wanted to do summation and filtering in the same step (again, I don't recommend it) you could use a List<String> to store the names, and never store the total prices anywhere except the loop-local variable totalPrice.
• Jack: the condition of equalsignorecase is used to avoid repetition.
• Jack: In the new code you also removed the condition of being more than target amount
• Chris Hayes: @Jack Yes, that was the point. That condition can be in a separate block below what I've posted, which separates the two actions of calculating the total price and filtering entries based on that price.

Answer 2

Now that we have a bit more details compared to your initial question, here's how I'll roughly separate the code...

1. Reading from file (input)

In your example, you have a long String declared as factories, which is comma-delimited. Does that imply your input is from a CSV file? If so, you should read up on external third-party libraries that can read a line from a CSV file correctly (i.e. taking into consideration escaped commas, quotes, maybe even do some light data type conversion) and return you a meaningful representation, such as a List of String fields. Afterwards, you can extract the discrete field values to construct your Company objects.

2. Summing up values (processing)

Similar to my answer to your original question, you do not need two for loops to traverse and selectively update values in your map. It is easier to simply think of this step as nothing more than a summation of the map's values categorized by the brands, as recorded in your Company objects.

3. Filtering the output (output)

This is where you perform the filtering on the map's entries to selectively display the brands with ____ (price?) greater than your specified value of 10000.

Putting it together

An example class structure can be as such:

public class CarReport {
 private static List<String> readFile(final String fileName) {
 // use a CSV library to parse the file
 return rows;
 }
 private static List<Company> parseData(final List<String> rows) {
 // convert "car,price" into a new Company(car, price)
 return companyList;
 }
 private static Map<String, BigDecimal> sumPricesByBrand(final List<Company> companyList) {
 // loop through companyList and incrementally add the prices to the map
 return map;
 }
 private static void displayFilteredOutput(final Map<String, BigDecimal> map, final BigDecimal threshold) {
 for (Entry<String, BigDecimal> entry : map.entrySet()) {
 // if entry.getValue() is more than threshold, display it
 }
 }
 public static void main(String[] args) {
 // example
 displayFilteredOutput(sumPricesByBrand(parseData(readFile("/path/to/input.csv"))), new BigDecimal("10000"));
 }
}