5
\$\begingroup\$

Could you review the approach and let me know if there is a better way to implement the same using different java techniques (also performant)?

Requirement as follows:

The products that are returned for a request, need to be ranked to offer the most relevant product to the user. we will rank based on weight and then on pricePerWeight.

The products will be ranked on weight first, and grouped in the following categories:

A: Products with an exact weight match as the request (Best match)

B: Products with a weight that is higher than the request (Second best)

C: Products with a weight that is lower than the request (Third best) 

D: Products with a different weight measure than the request (Worst match)

After the grouping by weight, the products in each group are ranked by pricePerWeight, ascending. Products that are a group in a particular group (for example group C) can never score higher than a product from a higher group (for example B).

   import java.util.Arrays;
    import java.util.Collection;
    import java.util.Comparator;
    import java.util.List;
    import java.util.stream.Collectors;
    import java.util.stream.Stream;

    public class Test {

        public static void main(String[] arrays) {

            final String unit = "grams";
            final double quantity = 500;

            final Sku sku8 = new Sku("123456", 800, "grams", 4);
            final Sku sku1 = new Sku("234567", 600, "grams", 5);
            final Sku sku2 = new Sku("354635", 500, "grams", 7);
            final Sku sku3 = new Sku("345666", 500, "grams", 4);
            final Sku sku4 = new Sku("234545", 400, "grams", 5);
            final Sku sku6 = new Sku("765434", 700, "milliliter", 4);
            final Sku sku5 = new Sku("765434", 500, "milliliter", 3);
            final Sku sku7 = new Sku("765434", 600, "milliliter", 5);

            final List<Sku> skus = Arrays.asList(sku8, sku1, sku2, sku3, sku4, sku5, sku6, sku7);

            final List<Sku> bestMatch = skus.stream()
                .filter(sku -> unit.equalsIgnoreCase(sku.getUnit()) && Double.compare(quantity, sku.getQuantity()) == 0)
                .sorted(Comparator.comparing(Sku::getPricePerWeight))
                .collect(Collectors.toList());

            final List<Sku> secondBest = skus.stream()
                .filter(sku -> unit.equalsIgnoreCase(sku.getUnit()) && Double.compare(quantity, sku.getQuantity()) == -1)
                .sorted(Comparator.comparing(Sku::getPricePerWeight))
                .collect(Collectors.toList());

            final List<Sku> thirdBest = skus.stream()
                .filter(sku -> unit.equalsIgnoreCase(sku.getUnit()) && Double.compare(quantity, sku.getQuantity()) == 1)
                .sorted(Comparator.comparing(Sku::getPricePerWeight)).collect(Collectors.toList());

            final List<Sku> worstMatch = skus.stream()
                .filter(skuToProcess -> !unit.equalsIgnoreCase(skuToProcess.getUnit()))
                .sorted(Comparator.comparing(Sku::getPricePerWeight))
                .collect(Collectors.toList());

            final List<Sku> sortedSkus = Stream.of(bestMatch, secondBest, thirdBest, worstMatch)
                .flatMap(Collection::stream)
                .collect(Collectors.toList());

            System.out.println("******Before sort*****");
            skus.forEach(skuToPrint -> System.out.println("ID:" + skuToPrint.getId() + " quantity: " + skuToPrint.getQuantity() + " uom: " + skuToPrint.getUnit() + " price: " + skuToPrint.getPricePerWeight()));

            System.out.println("******sortedSkus*****");
            sortedSkus.forEach(skuToPrint -> System.out.println("ID:" + skuToPrint.getId() + " quantity: " + skuToPrint.getQuantity() + " uom: " + skuToPrint.getUnit() + " price: " + skuToPrint.getPricePerWeight()));

}

Sku class

public class Sku {

    private String id;
    private double quantity;
    private String unit;
    private double pricePerWeight;

    public String getId() {
        return id;
    }

    public double getQuantity() {
        return quantity;
    }

    public String getUnit() {
        return unit;
    }

    public double getPricePerWeight() {
        return pricePerWeight;
    }

    public Sku(String id, double quantity, String unit, double pricePerWeight) {
        this.id = id;
        this.unit = unit;
        this.quantity = quantity;
        this.pricePerWeight = pricePerWeight;
    }
}
\$\endgroup\$
5
  • \$\begingroup\$ I see a variable called pricePerWeight, where does weight come in, is it quanity? \$\endgroup\$
    – pacmaninbw
    Dec 11, 2019 at 13:51
  • \$\begingroup\$ "and grouped in the following categories:" is part of your requirements. Does this mean they must be grouped into 4 lists and you cannot use a single sorting method? \$\endgroup\$
    – dustytrash
    Dec 11, 2019 at 14:50
  • \$\begingroup\$ @pacmaninbw: pricePerWeight comes from a backend system and this has a different price than the price of the product itself. \$\endgroup\$ Dec 11, 2019 at 17:42
  • \$\begingroup\$ @dustytrash, I did try to use a Comparator and that did not succeed, hence I came up with the above solution. Though I did not try using a single list to do all the above actions and retrieve, but if we have a different list, it is clear to understand and read on what is captured on it and sorted after that ! \$\endgroup\$ Dec 11, 2019 at 17:42
  • \$\begingroup\$ Welcome to Code Review! Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$
    – Vogel612
    Dec 13, 2019 at 13:14

2 Answers 2

2
\$\begingroup\$

1) First I suggest that you extract the commons predicates into methods; theres lots of duplicated code.

    //[...]
    final List<Sku> bestMatch = getSkuByWeight(skus.stream(), getSkuFilterPredicateByWeight(unit, quantity, 0));
    final List<Sku> secondBest = getSkuByWeight(skus.stream(), getSkuFilterPredicateByWeight(unit, quantity, -1));
    final List<Sku> thirdBest = getSkuByWeight(skus.stream(), getSkuFilterPredicateByWeight(unit, quantity, 1));
    final List<Sku> worstMatch = getSkuByWeight(skus.stream(), skuToProcess -> !unit.equalsIgnoreCase(skuToProcess.getUnit()));
    //[...]

    private static <T> List<Sku> getSkuByWeight(Stream<Sku> stream, Predicate<Sku> predicate) {
        return stream.filter(predicate).sorted(Comparator.comparing(Sku::getPricePerWeight)).collect(Collectors.toList());
    }

    private static Predicate<Sku> getSkuFilterPredicateByWeight(String unit, double quantity, int toCompare) {
        return sku -> unit.equalsIgnoreCase(sku.getUnit()) && Double.compare(quantity, sku.getQuantity()) == toCompare;
    }

2) You can reuse the string unit in the creation of the Sku objects.

    //[...]
    final Sku sku8 = new Sku("123456", 800, unit, 4);
    final Sku sku1 = new Sku("234567", 600, unit, 5);
    final Sku sku2 = new Sku("354635", 500, unit, 7);
    final Sku sku3 = new Sku("345666", 500, unit, 4);
    final Sku sku4 = new Sku("234545", 400, unit, 5);
\$\endgroup\$
1
  • \$\begingroup\$ Thank you for taking the time to review firstly. First one looks possible with the real implementation. Second one can't, because the unit used there for test purpose. \$\endgroup\$ Dec 11, 2019 at 18:50
2
\$\begingroup\$

Overriding toString

Inside your Sku class, to print useful info instead of constructing the information outside of the class:

@Override
public String toString() {
    return "ID:" + getId() + " quantity: " + getQuantity() + " uom: " + getUnit() + " price: " + getPricePerWeight();
}

Then your print statements would look like:

System.out.println("******Before sort*****");
skus.forEach(skuToPrint -> System.out.println(skuToPrint));
System.out.println("******sortedSkus*****");
sortedSkus.forEach(skuToPrint -> System.out.println(skuToPrint));

Change variable name 'arrays'

'args' is pretty standard for command line arguments. 'Arrays' doesn't make sense anyway since it's 1 array

public static void main(String[] arrays) {

// change to:
public static void main(String[] args) {

Take advantage of Junit tests

It's more clear without running your code & no more manually reading the output:

public class SkuTest {
    @Test
    public void testSkuCompareQuantity() {
        final Sku sku8 = new Sku("123456", 800, "grams", 4);
        final Sku sku1 = new Sku("234567", 600, "grams", 5);
        final Sku sku2 = new Sku("354635", 500, "grams", 7);
        final Sku sku3 = new Sku("345666", 500, "grams", 4);
        final Sku sku4 = new Sku("234545", 400, "grams", 5);
        final Sku sku6 = new Sku("765434", 700, "milliliter", 4);
        final Sku sku5 = new Sku("765434", 500, "milliliter", 3);
        final Sku sku7 = new Sku("765434", 600, "milliliter", 5);

        final List<Sku> expectedBestResults = Arrays.asList(sku2, sku3);
        final List<Sku> expectedSecondBestResults = Arrays.asList(sku1, sku8);
        final List<Sku> expectedThirdBestResults = Arrays.asList(sku4);
        final List<Sku> worstResults = Arrays.asList(sku6, sku5, sku7);

        final List<Sku> skus = Arrays.asList(sku8, sku1, sku2, sku3, sku4, sku5, sku6, sku7);
        final List<Sku> sortedSkus = SkuSorter.sortSkuList(skus);

        // assert best results
        assertTrue(sortedSkus.containsAll(expectedBestResults));

        // assert second best results
        assertTrue(sortedSkus.containsAll(expectedSecondBestResults));

        // assert third best results
        assertTrue(sortedSkus.containsAll(expectedThirdBestResults));

        // assert worst results
        assertTrue(sortedSkus.containsAll(worstResults));

        // sanity check
        assertEquals(skus.size(), sortedSkus.size());
    }
}

Note: I separated the logic into another class.

Consider using a Comparator or CompareToBuilder

import java.util.Comparator;
import org.apache.commons.lang3.builder.CompareToBuilder;

public class SkuComparator implements Comparator<Sku> {
    private final String unit;
    private final double quantity;

    public SkuComparator(String unit, double quantity) {
        super();

        this.unit = unit;
        this.quantity = quantity;
    }

    @Override
    public int compare(Sku sku1, Sku sku2) {
        return new CompareToBuilder().append(unit.equalsIgnoreCase(sku1.getUnit()), unit.equalsIgnoreCase(sku2.getUnit()))
                .append(Double.compare(quantity, sku1.getQuantity()), Double.compare(quantity, sku2.getQuantity()))
                .toComparison();
    }
}

Notes: tested using above test, it all passes.

No need to sort by 'worst match', as that already comes last.

Using the comparator you can sort using: skus = skus.stream().sorted(new SkuComparator(unit, quantity)).collector(Collectors.toList());

\$\endgroup\$
1
  • \$\begingroup\$ Really appreciate your inputs. But the above solution looks to me like not solving the issue entirely, As the grouping works by weight as per the requirement, but not the price sorting as intended. I did put weight grouping in a code block and the price sorting beneath that block, might be that have caused for you to miss that statement. I will change it to as a code block. \$\endgroup\$ Dec 13, 2019 at 11:38

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.