2
\$\begingroup\$

I got the following task to solve:

"PRODUCT_NAME","PRODUCT_CATEGORY","SALES_PRICE"
"Hair Gel","Beauty",6.99
"Scotch tape","Office Supply",2.99
"Office Chair","Office Supply",134.99
"Eraser","Office Supply",0.99
"Lipstick","Beauty",9.99
"Nail Polish","Beauty",8.99
"Note Book","Office Supply",2.99
"Flower Pot","Home & Gardening",22.76
"Garden Hose","Home & Gardening",12.87

This format is referred to as a CSV (Comma Separated Values). Review this and understand what kind of data it is. The first line is called the header. The lines following the header is the actual data in the respective order. Each attribute is separated with a comma hence the name.

Requirements:

To work on this assignment, first you need to create a file called products.csv in your eclipse workspace and paste the above data in that file.

Your program should read through the file and compute the following calculations.

  1. The average price for each product_category.

  2. The total sales value for each product_category.

  3. The number of products in each product_category.

  4. The most expensive item in each product_category.

Your program should print to the screen the information. Here is an example of how it should be printed out:

Beauty
----------------------------------
average_price = 8.66
total_sales_value = 25.97
number_of_products = 3
most_expensive = 9.99

Office Supply
----------------------------------
average_price = xxxx

etc etc...

Here is how i solved it:

//AggregateUtils

public class AggregateUtil {
//static methods that do the calculations..


    static double getTheAveragePriceForEachCategory(List<Double> product){
        double average = getTotalSalesValueForEachCategory(product)/(double)getNumberOfProductsInEachCategory(product);
        return Math.round(average * 100D) / 100D;
    }

    static double getTotalSalesValueForEachCategory(List<Double> product){
        double sum = 0.0;
        for(Double value : product){
            sum+=value;
        }
        return Math.round(sum * 100D) / 100D;
    }

    static int getNumberOfProductsInEachCategory(List<Double> product){
        return (product.size());
    }

    static double getTheMostExpensiveProductInEachCategory(List<Double> product){
        double highest = Double.MIN_VALUE;
        for(Double value : product){
            if(value>highest) highest = value;
        }
        return highest;
    }
}

//DisplayUtils

public class DisplayUtil {
//prints the values under each product category..
    public static void DisplayProducts( Map<String, List<Double>> db){
        StringBuilder builder = new StringBuilder();
        for (Map.Entry<String,List<Double>> entry : db.entrySet()) {
              builder = new StringBuilder();
              System.out.println(entry.getKey());
              System.out.println("----------------------------------");
              builder.append("average_price = ");
              double avrg_price = AggregateUtil.getTheAveragePriceForEachCategory(entry.getValue());
              builder.append(avrg_price);
              builder.append("\n");
              builder.append("total_sales_value = ");
              double total_sales = AggregateUtil.getTotalSalesValueForEachCategory(entry.getValue());
              builder.append(total_sales);
              builder.append("\n");
              builder.append("number_of_products = ");
              double numberOfProducts = AggregateUtil.getNumberOfProductsInEachCategory(entry.getValue());
              builder.append(numberOfProducts);
              builder.append("\n");
              builder.append("most_expensive = ");
              double mostExpensive = AggregateUtil.getTheMostExpensiveProductInEachCategory(entry.getValue());
              builder.append(mostExpensive);
              builder.append("\n");
              System.out.println(builder + "\n");

            }
    }
}

//FileUtils

public static void ReadFile(String path, Map<String, List<Double>> db){
    try(BufferedReader br = new BufferedReader(new FileReader(path))){
        String line;
        if((line = br.readLine()) != null){
            //this is the first heading..and needed to be skipped
        }
        while ((line = br.readLine()) != null) {
            String[] keys = line.split(",");
            keys[1] = keys[1].replace("\"", "");
            if(db.containsKey(keys[1])){
                List<Double> list = db.get(keys[1]);
                list.add(Double.valueOf(keys[2]));
                db.put(keys[1], list);

            }
            else{
                List<Double> list = new ArrayList<>();
                list.add(Double.valueOf(keys[2]));
                db.put(keys[1],list);
            }
        }

    }catch(IOException e){}
}


}

//the controller class

public class ControllerClass {

    public static void main(String []args){
        Map<String, List<Double>> db = new HashMap<>();
        FileUtils.ReadFile("./Files/products.csv", db);
        DisplayUtil.DisplayProducts(db);

    }

}

I just need to get your feedback on areas to improve in the coding.

\$\endgroup\$
4
\$\begingroup\$

Java Arrays

In ControllerClass, I would rather use the following signature for main():

public static void main(final String... args)

I find it important that in Java the [] or ... is written with the type, not with the variable, because being an array is - in Java (unlike C) - an attribute of the type, not the variable, therefore i.e. String[] args and not String []args or String args[].

Using ... instead of [] is useful for testing, because you do not need to wrap the String arguments into a String[] yourself, the VM will do it for you.

Java Naming Convention

Method names in Java start with lower case letters (unlike C#). So, rename:

ReadFile -> readFile
DisplayProducts -> displayProducts

Use a local instead of calling entry.getValue() multiple times.

I'm not saying this due to performance reasons. It makes the code easier to refactor and easier to read.

When you extract the body of this loop into a separate method, you would end up with methods that have an unnatural dependency on Map.Entry:

for (final Map.Entry<String, List<Double>> entry : map) {
    action1(entry.value());
    action2(entry.value());
    action3(entry.value());
    action4(entry.value());
}

Instead use

for (final Map.Entry<String, List<Double>> entry : map) {
    final List<Double> values = entry.value();
    action1(values);
    action2(values);
    action3(values);
    action4(values);
}

For output, prefer System.lineSeparator() over "\n"

When you hard-code "\n", the file is not correct for Windows, and Windows is still around in relevant numbers. (See also next point.)

In your case, prefer Formatter over StringBuilder

Using final Formatter f = new Formatter(); would significantly simplify your code. Together with import static for the static methods of the other class and having extracted entry.getValue() into a separate variable values, it could look like this:

f.format("average_price = %f%n", getTheAveragePriceForEachCategory(values));

Double Literals D vs d vs .0

Regarding double literals, 1000D is correct, but in some fonts 0 can be a bit difficult to distinguish from D. It would be better to write 1000d. Best would be 1000.0 because that's what most people use and I can't see any good argument against it.

Use the right names

For example, in static double getTotalSalesValueForEachCategory(List<Double> product) the parameter product does not contain a product, it contains various prices of a product and thus should be named productPrices.

Consider using Java 8 Streams

For example, getTotalSalesValueForEachCategory could be written, using Streams, like this:

static double getTotalSalesValueForEachCategory(final List<Double> productPrices) {
    return productPrices
        .stream()
        .mapToDouble(Double::doubleValue)
        .sum() / 100.0;
}

Avoid casts which are redundant with type promotion.

In

static double getTheAveragePriceForEachCategory(List<Double> product){
    double average = getTotalSalesValueForEachCategory(product)/(double)getNumberOfProductsInEachCategory(product);
    return Math.round(average * 100D) / 100D;
}

The cast to (double) is unnecessary as the return type of getTotalSalesValueForEachCategory() already is double, and for double / int the int is promoted to double.

Write smaller methods (SRP - Single Responsibility Principle for methods)

Good methods do one thing, they do it well and they do it only. They operate on only one or at maximum two levels of abstraction, and out of the set of constructs sequence, condition, iteration, exception handling, synchronization they use at best only one, at max two and only in rare cases three.

DisplayProducts is hard to test

It's not impossible to test, but in order to test it, you would need to redirect System.out using System.setOut(). This is possible, but effort that can be easily avoided when you split the responsibility: DisplayProducts currently is operating on three levels of abstraction, and doing three things:

  • Formatting the parts of the report
  • Generating the report
  • Printing the report

The printing could easily be moved from DisplayProducts to main().

Avoid Util as a class name.

Usually, class names like Util, Manager, Info or Data are a sign of bad design. In your case, the class could be named ProductsReporter.

Maybe use more OO

This may be the most important part of this entire review.

Your program is written in a completely non-object-oriented style. Your classes are already quite nicely separated into different responsibilities. However, your classes only encapsulate operations. They do not encapsulate the data on which these operations are performed.

That you pass around multiple times Collections which carry more semantics than just holding random data is a sign of missing classes or missing encapsulation of data.

Finish what you start

ReadFile forgets to close the file that it opened. I recommend to use Java 7 try-with-resources.

Don't silently ignore exceptions, especially checked exceptions

When the file couldn't be opened, there would be no message for the user to tell what happened. The program would just continue and print nothing.

Further separate responsibilities

The generation of CSV should be a separate responsibility in order to handle special situations like Strings which contain ". It might make sense to use one of the existing CSV libraries, or if you're interested in the topic and exercise, you could develop your own.

Map usage in ReadFile

The Map usage in ReadFile could be improved.

In the first branch, when the List already is in the Map, putting it back in is redundant.

In ReadFile. you know that every key you access shall exist. You could use computeIfAbsent(), like this:

map.computeIfAbsent(key, key -> ArrayList::new).add(Double.valueOf(value));
\$\endgroup\$
  • \$\begingroup\$ Thanks. on the side, how best can one send in a code for review? Imagine I have 10 files, I want to send in for reviews, this might be time consuming like I have done it now. Is there a better way to send in a jar file that can be reviewed in it's entirety? \$\endgroup\$ – Don Who Feb 13 '16 at 21:41
  • \$\begingroup\$ That's an excellent question. I think codereview.stackexchange.com has its limits for exactly the reason that you mentioned. That's a personal opinion. I haven't looked for better sites, maybe that's a "gap". \$\endgroup\$ – Christian Hujer Feb 13 '16 at 21:43
  • \$\begingroup\$ @ChristianHujer, Thanks. Also, I tried the java8 recommendation on my code but got the error: "The method mapToDouble(Double::doubleValue) is undefined for the type List<Double>" Oh, never mind, I got it figured out now. \$\endgroup\$ – Don Who Feb 13 '16 at 22:03
3
\$\begingroup\$

In addition to @Christian Hujer's answer...

Relying on return values than 'result' objects

Map<String, List<Double>> db = new HashMap<>();
FileUtils.ReadFile("./Files/products.csv", db);
// DisplayUtils over DisplayUtil for consistency and convention
DisplayUtils.DisplayProducts(db);

This can be simplified to the following if you rely on FileUtils.readFile() returning the output, instead of putting them into db:

DisplayUtils.displayProducts(FileUtils.readFile("./Files/products.csv"));

StringBuilder usage

When StringBuilders are used in looping and collecting String outputs, only one instance is created. For your usage, you are creating one instance per iteration just to print the results at the end of it, so that is slightly self-defeating. It will be easier, and likely clearer too, to just print them as such:

for (Map.Entry<String,List<Double>> entry : db.entrySet()) {
    System.out.println(entry.getKey());
    System.out.println("----------------------------------");
    System.out.println("average_price = " +
                AggregateUtil.getTheAveragePriceForEachCategory(entry.getValue()));
    System.out.println("total_sales_value = " +
                AggregateUtil.getTotalSalesValueForEachCategory(entry.getValue()));
    System.out.println("number_of_products = " +
                AggregateUtil.getNumberOfProductsInEachCategory(entry.getValue()));
    System.out.println("most_expensive = " +
                AggregateUtil.getTheMostExpensiveProductInEachCategory(entry.getValue()));
}

Java 8

There's a much better way to process lines of CSV data, when you rely on Java 8 stream-based processing. The implementation is much simpler (you write less code), and you get your results in a single pass of the data, instead of having to loop per result:

private static Map<String, DoubleSummaryStatistics> summarize(Collection<String> lines) {
    return lines.stream()
            .map(row -> row.split(","))
            .collect(Collectors.groupingBy(data -> data[1],
                    Collectors.summarizingDouble(data -> Double.valueOf(data[2]))));
}
  1. For each Stream<String> element (i.e. row of CSV data), map() into a String[] array by splitting on ",". This is assuming your CSV data is clean enough to be parsed in the simplest way possible, else you might want to consider using a CSV library to do this for you.
  2. Collect the results groupingBy() our category column (data[1]) and then summarize on our price column (data[2]).

To print from the resulting Map<String, DoubleSummaryStatistics> object, you just need a method accepting both String, DoubleSummaryStatistics arguments, so that you can use it as a BiConsumer in a forEach():

private static final Format FORMATTER = new DecimalFormat("#.##");

private static void printNumber(String description, double value) {
    System.out.printf("%20s: %s%n", description, FORMATTER.format(value));
}

private static void print(String category, DoubleSummaryStatistics summary) {
    System.out.println("Category: " + category);
    printNumber("Average price", summary.getAverage());
    printNumber("Total", summary.getSum());
    printNumber("Number of products", summary.getCount());
    printNumber("Most expensive", summary.getMax());
    System.out.println("-----");
}

// assuming class is called ProductSummarizer
public static void main(String[] args) {
    summarize(parseToCollection("/path/to/file")).forEach(ProductSummarizer::print);
}

Here, print(String, DoubleSummaryStatistics) is used as a method reference, and a DecimalFormat instance is used to format the numeric values.

More OOP

As briefly touched on by @Christian Hujer, we can improve on the summarize() method by having domain classes, instead of dealing with plain Strings. For example, with a Product class and some suitable methods, the method can be expressed in an even clearer manner:

private static Map<String, DoubleSummaryStatistics> summarize(
        Collection<Product> products) {
    return products.stream()
            .collect(Collectors.groupingBy(Product::getCategory,
                    Collectors.summarizingDouble(Product::getPrice)));
}

Putting it altogether

As commented by @Christian Hujer, once you get to stream processing on the file directly, a possible solution can be just:

public static void main(String[] args) throws IOException {
    try (Stream<String> lines = Files.lines(pathToCsvFile)) {
        lines.skip(1) // to skip the header row
                .map(ProductSummarizer::parse) // String -> Product function
                .collect(Collectors.groupingBy(Product::getCategory,
                        Collectors.summarizingDouble(Product::getPrice)))
                .forEach(ProductSummarizer::print);
    }
}

Here, we use Files.lines(Path) to get our Stream, and we can inline the processing within summarize() too.

\$\endgroup\$
  • \$\begingroup\$ I disagree with your comment on StringBuilder usage. Formatting and Printing should be separated because that makes it easier to test. Also, writing text in larger chunks makes programs behave nicer and logs easier to read when multiple processes write into the same file. Still a +1 for all the other nice things :) \$\endgroup\$ – Christian Hujer Feb 14 '16 at 10:25
  • 1
    \$\begingroup\$ Regarding Java 8: And it will be even shorter when using a Stream<String> of lines in the first place, as offered by the Java 8 API of BufferedReader and Paths. \$\endgroup\$ – Christian Hujer Feb 14 '16 at 10:30
  • \$\begingroup\$ @ChristianHujer fair point regarding StringBuilder, in that case how the scoping of builder in OP's code can be improved then. :) Added a final section based on your comments too, about the use of Stream. \$\endgroup\$ – h.j.k. Feb 14 '16 at 13:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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