5
\$\begingroup\$

I've been trying to learn Groovy lately and I tried to solve a problem which involves designing a routine that will calculate the average Product price per Group.

I have the following data:

// contains information about [Product, Group, Cost]
products = [
    ["A", "G1", 20.1],
    ["B", "G2", 98.4],
    ["C", "G1", 49.7],
    ["D", "G3", 35.8],
    ["E", "G3", 105.5],
    ["F", "G1", 55.2],
    ["G", "G1", 12.7],
    ["H", "G3", 88.6],
    ["I", "G1", 5.2],
    ["J", "G2", 72.4]
]

// contains information about Category classification based on product Cost
// [Category, Cost range from (inclusive), Cost range to (exclusive)]
// i.e. if a Product has Cost between 0 and 25, it belongs to category C1
category = [
    ["C3", 50, 75],
    ["C4", 75, 100],
    ["C2", 25, 50],
    ["C5", 100, null],
    ["C1", 0, 25]
]

// contains information about margins for each product Category
// [Category, Margin (either percentage or absolute value)]
margins = [
    "C1" : "20%",
    "C2" : "30%",
    "C3" : "0.4",
    "C4" : "50%",
    "C5" : "0.6"]

What I tried so far is the following (I mixed some Java here):

def calculateResult() {
    def group1 = []
    def group2 = []
    def group3 = []
    def pricePerGroupMap = [:]

category.each { cat ->
    String catDesc = cat[0]
    BigDecimal min = cat[1]
    BigDecimal max = cat[2]

    if (max == null) {
        max = 9999.9;
    }

    products.each { product ->
        BigDecimal currProductPrice = BigDecimal.valueOf(product[2])
        if (currProductPrice.compareTo(min) >= 0 && currProductPrice.compareTo(max) <= 0) {
            String selectedMarginAsString = margins.getAt(catDesc);
            BigDecimal selMargin = 0.0;

            if (selectedMarginAsString.endsWith("%")) {
                selMargin = new BigDecimal(selectedMarginAsString.trim().replace("%", "")).divide(BigDecimal.valueOf(100));
            } else {
                selMargin = new BigDecimal(selectedMarginAsString);
            }

            if (product[1].equals("G1")) {
                group1.add(product[2] * (1 + selMargin))
            } else if (product[1].equals("G2")) {
                group2.add(product[2] * (1 + selMargin))
            } else if (product[1].equals("G3")) {
                group3.add(product[2] * (1 + selMargin))
            }
        }

    }
}
pricePerGroupMap["G1"] = group1.sum() / group1.size()
pricePerGroupMap["G2"] = group2.sum()/ group2.size()
pricePerGroupMap["G3"] = group3.sum()/ group3.size()
print pricePerGroupMap
}

I am sure there are better ways to optimize this algorithm taking advantage of closures.

\$\endgroup\$

2 Answers 2

3
\$\begingroup\$

This is already old and your coding skills have probably improved enough since then that you no longer need a review, but I just couldn't resist revising the code. May it help others appreciate Groovy.

Main changes:

  • introduced two classes Product and Category for better readability and transformed the lists accordingly. The annotation @Canonical provides cool functionality like a constructor and a meaningful toString() method.
  • completely changed the calculateResult.

Method calculateResult

  • Within the method, it makes much more sense to first group the products by their groups and then iterate over their products per group than to start iterating by categories.
  • I put the categories into a TreeMap to find the right Category for a Product. This may seem a bit over the top, but it not only improves the time complexity for one lookup of a Category for a given Product from O(n) to O(log(n)), where n is the number of categories, but also is very convenient to use here. The floorEntry method on a TreeMap

returns a key-value mapping associated with the greatest key less than or equal to the given key, or null if there is no such key

  • The method average() came with Groovy 3.

Codestyle

  • If you don't use def or the type before the variable name when declaring variables, the variable will be added to the script's binding. It is more readable and better for encapsulation to define them the usual way and then pass them to the method explicitly.
  • Although Groovy ignores generics in Maps and Lists, I'd advise including them for documentation purposes if it's not obvious. Your colleagues and your future self will appreciate that. In addition, you help your IDE to support you.
import groovy.transform.Canonical

List<Product> products = [
    ["A", "G1", 20.1],
    ["B", "G2", 98.4],
    ["C", "G1", 49.7],
    ["D", "G3", 35.8],
    ["E", "G3", 105.5],
    ["F", "G1", 55.2],
    ["G", "G1", 12.7],
    ["H", "G3", 88.6],
    ["I", "G1", 5.2],
    ["J", "G2", 72.4]
].collect { new Product(*it) }

Map<String, String> margins = [
    "C1" : "20%",
    "C2" : "30%",
    "C3" : "0.4",
    "C4" : "50%",
    "C5" : "0.6"
]

List<Category> categories = [
        ["C3", 50, 75],
        ["C4", 75, 100],
        ["C2", 25, 50],
        ["C5", 100, null],
        ["C1", 0, 25]
].collect{ new Category(*it, Category.parseMargin(margins[it[0]])) }

Map<String, BigDecimal> avgPricePerGroupMap = calculateResult(products, categories)
assert avgPricePerGroupMap == [G1:37.498, G2:124.48, G3:116.08]

Map<String, BigDecimal> calculateResult(List<Product> products, List<Category> categories) {
    TreeMap<Integer, Category> cats = new TreeMap(categories.collectEntries { Category cat -> [cat.minPrice, cat] })

    products.groupBy { Product p -> p.group }
            .collectEntries { String group, List<Product> ps ->
                List<BigDecimal> prices = ps.collect { Product p -> p.price * (1 + cats.floorEntry(p.price).value.margin) }
                [group, prices.average()]
            }
}

@Canonical
class Product {
    String name
    String group
    BigDecimal price
}

@Canonical
class Category {
    String id
    BigDecimal minPrice
    BigDecimal maxPrice
    BigDecimal margin

    static BigDecimal parseMargin(String margin){
        return margin.endsWith("%") ? margin[0..-2].toBigDecimal() / 100 : margin.toBigDecimal()
    }
}
```
\$\endgroup\$
1
\$\begingroup\$

After being investigating a little bit more about usage of closures and some other facilities that Groovy provides, I managed to reduce a little bit the algorithm, after looking for benchmarking posts, I believe this is probably not the most optimal solution, but it looks a lot better than the first proposal (the one in my question).

One of the things I considered to improve the solution is to use Groovy "maps" instead of multiple arrays to hold the data.

I also considered using one additional variable to hold the value of a group being evaluated.

And finally, I decided to avoid using more conditionals to add the results to the arrays and use a more simple instruction taking advantage of the map definition.

MAX_DEFAULT_VALUE_WHEN_NULL = 99999.9

def calculateResult() {
    def pricesPerGroupMap = [G1:[], G2:[], G3:[]]
    def avgPricePerGroupMap = [:]

    category.each { cat ->
        String categoryId = cat[0]
        BigDecimal min = cat[1]
        BigDecimal max = cat[2]

        if (max == null) {
            max = MAX_DEFAULT_VALUE_WHEN_NULL
        }

        products.each { product ->
            BigDecimal currProductPrice = BigDecimal.valueOf(product[2])
            String group = product[1]
            if (currProductPrice.compareTo(min) >= 0 && currProductPrice.compareTo(max) == -1) {
                String selectedMarginAsString = margins.getAt(categoryId);
                BigDecimal selMargin = 0.0;

                if (selectedMarginAsString.endsWith("%")) {
                    selMargin = new BigDecimal(selectedMarginAsString.trim().replace("%", "")).divide(BigDecimal.valueOf(100))
                } else {
                    selMargin = new BigDecimal(selectedMarginAsString)
                }
                pricesPerGroupMap[group].add(currProductPrice * (1 + selMargin))
            }
        }
    }
    pricesPerGroupMap.each { k, v -> 
        avgPricePerGroupMap.put(k, new BigDecimal(v.sum()).divide(v.size(), 1, RoundingMode.UP))
    }
    return avgPricePerGroupMap
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Hi Marcelo! Can you edit your post to provide some more detail about what makes this answer better than the original code posted in the question? \$\endgroup\$
    – cariehl
    Commented May 9, 2019 at 17:05
  • 1
    \$\begingroup\$ Sure @cariehl, thanks for the heads up. I added more detail on the changes I made and how I implemented them. Please, let me know if more details are needed for this post. Thanks in advance. \$\endgroup\$ Commented May 9, 2019 at 20:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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