Sorting a list and another list inside each item

I have a list of factories. Each factory has an item of its own and a list of other items from competitors.

I need to sort the list of factories based on price of their items and also sort list of other items from competitors for each factory.

What I am doing require to sort collection of factories and loop through all factories and sort collection of their competitors. I am wondering if there is any easier way to do it.

Factory

class Factory implements Comparator{

String item;
double price;
List competitors;
@Override
    public int compare(Factory o1, Factory o2) {
        if (o1.getPrice()  o2.getPrice())
            return 1;
        return 0;
    }}

Competitor

class Competitor implements Comparator{
String facName;
double price;
@Override
public int compare(Competitor o1, Competitor o2) {
        if (o1.getPrice()  o2.getPrice())
            return 1;
        return 0;
}

Sort

Collection.sort(factoriesList); //sort list of factories
//sort list of competitors of each factory
for(int i=0;i<factoriesList.size();i++){
   Collection.sort(factoriesList.getCometitors());
}

Sample result

Factory1 Item1 222  Com1 444 Com2 555
Factory2 Item2 223  Com3 555 Com4 676
Factory3 Item3 224  Com1 543 
Factory4 Item4 225  Com9 322

There are others concerns with your code, without going into the sort:

Getter returning mutable data

getCompetitors() returns directly the internal list stored by your factory object. This is generally not a good idea: it means a client of Factory can modify its internal structure, which defeats the OOP principle. It would be preferable instead to have a method sortCompetitors(), that would sort the list, without leaking it:

private List competitors;

public void sortCompetitors() {
    Collections.sort(competitors);
}

and remove completely the method getCompetitors().

Comparator, Comparable

class Factory implements Comparator

will be problematic in the future. It seems what you want would be to use Comparable instead, but even this isn't a good idea in this case. There is a difference between the two: a class is Comparable when it can compare itself to another class of the same type, which is what you are doing here: one Factory is comparing itself to another object. On the other hand, a Comparator is a class that is comparing 2 objects of the same type (it does not compare this with another object).

Therefore:

• It is wrong to have your class implement Comparator: it means the class is doing too much: it knows itself and it is also responsible for comparing any 2 factories (See also on Stack Overflow). More often than not, there are several ways to compare 2 objects given a context: sort by name or sort by price...

• It is also probably wrong to have your class implements Comparable, but for a different reason. As said before, Comparable is used to compare this with another object of the same type. But this interface is closely related to equals and hashCode: any class that is Comparable should provide a consistent equals method (It is strongly recommended, but not strictly required that (x.compareTo(y)==0) == (x.equals(y))). In your case, it would imply that two factories are equal when their price is equal, which is probably not what you want.

The solution here is not to make your class implements Comparator and define a custom comparator class, like

class FactoryPriceComparator implements Comparator

Using built-in methods

Your compare methods are currently doing:

@Override
public int compare(Factory o1, Factory o2) {
    if (o1.getPrice()  o2.getPrice())
        return 1;
    return 0;
}

This can be written more concisely with the built-in Double.compare (since Java 7), which also properly handles NaN, -0.0 and 0.0, contrary to your current code:

public int compare(Factory o1, Factory o2) {
    return Double.compare(o1.getPrice(), o2.getPrice());
}

Your whole comparator would then become:

public class FactoryPriceComparator implements Comparator {
    @Override
    public int compare(Factory o1, Factory o2) {
        return Double.compare(o1.getPrice(), o2.getPrice());
    }
}

Note that you would have the same implementation for the Comparator.

Java 8 API

If you're using Java 8, you can even get rid of the above FactoryPriceComparator and use the built-in Comparator.comparingDouble(keyExtractor), which creates a comparator comparing the double values returned by the key extractor. In this case, the key extractor could be the method reference Factory::getPrice (resp. Competitor::getPrice). So you could simply have:

Collections.sort(factoriesList, Comparator.comparingDouble(Factory::getPrice));

or even

factoriesList.sort(Comparator.comparingDouble(Factory::getPrice));

Finally:


What I am doing require to sort collection of factories and loop through all factories and sort collection of their competitors

This is actually the proper way of doing it: when you sort a Factory, you cannot sort the inner competitors at the same time, because different objects are being compared.

StackExchange Code Review Q#129547, answer score: 7