Sorting a list and another list inside each item

Question

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<Factory>{
String item;
double price;
List<Competitor> competitors;
@Override
 public int compare(Factory o1, Factory o2) {
 if (o1.getPrice() < o2.getPrice())
 return -1;
 if (o1.getPrice() > o2.getPrice())
 return 1;
 return 0;
 }}

Competitor

class Competitor implements Comparator<Competitor>{
String facName;
double price;
@Override
public int compare(Competitor o1, Competitor o2) {
 if (o1.getPrice() < o2.getPrice())
 return -1;
 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

• Vogel612: Do you have java 8 available?
• Bruno Costa: Did you mean Collection.sort(factoriesList.get(i).getCometitors());? was it a copy paste problem?

Answer 1

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<Competitor> competitors;
public void sortCompetitors() {
 Collections.sort(competitors);
}

and remove completely the method getCompetitors().

Comparator, Comparable

class Factory implements Comparator<Factory>

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<Factory>

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;
 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<Factory> {
 @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<Competitor>.

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.

• Jack: Thanks for your answer, I learned a lot. I am a bit confused with FactoryPriceComparator class. My question is how to call compare method of factoryPriceComparator to sort factories? Do I need to loop through them and pass them to the compare method?
• Bruno Costa: 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. This is only true if the returned list is not readonly.
• Tunaki: @Jack Yes, like what I did in the last example. You can have an instance of the comparator (let's call it factoryPriceComparator) and use it like: Collections.sort(factoriesList, factoryPriceComparator);. This will sort all factories according to their price. Then, yep, you need to loop through them and sort the competitors. You don't call directly the compare method, it will be called during the sort operation.
• Tunaki: @BrunoCosta Correct, I assumed it wasn't readonly since the OP called Collection.sort(factoriesList.getCometitors());, meaning that the getter returns the list itself. If there is such a getter, you'd want to wrap it with Collections.unmodifiableList for example.