ComperatorFactory using static methods or each in its own class?

Question

I was thinking of implementing multiple Comperators. Now I am not sure about how to do this in a most favourable way, i.e. still having the ability to do dependency injection.

Method 1: One class is one comparator (as seen in this answer):

// file LexicographicComparator.java
class LexicographicComparator implements Comparator<Person> {
    @Override
    public int compare(Person a, Person b) {
        return a.name.compareToIgnoreCase(b.name);
    }
}

// file AgeComparator.java
class AgeComparator implements Comparator<Person> {
    @Override
    public int compare(Person a, Person b) {
        return a.age < b.age ? -1 : a.age == b.age ? 0 : 1;
    }
}

Method 2: Now it would surely be possible to do sth like this:

class PersonComperatorFactory {
    public static Comparator<Person> getAgeComparator() {
        return new Comparator<Person>() {
            @Override
            public int compare(Person a, Person b) {
                return a.age < b.age ? -1 : a.age == b.age ? 0 : 1;
            }
        }
    }
    
    public static Comparator<Person> getLexicographicComparator() {
        return new Comparator<Person>() {
            @Override
            public int compare(Person a, Person b) {
                return a.name.compareToIgnoreCase(b.name);
            }
        }
    }
}

The second method is surely less cluttering your packages (put it to the .domain package or .util package? Or create a new .domain.comperators package?). But is this a good idea at all? I mean, does this have any benefits besides less files? What about code reusage, or DependencyInjection - may it have drawbacks I am yet unable to see?

I'd like this to be not a "chose your favourite"-question, but there may be many good reasons why to use one or the other.

Thank you very much in advance.

PS: In which package would you put this (this is probably a personal choice)? But this is only a side-question suitable for a comment.

I wouldn't use a factory class unless there were a good reason for it. You don't have one (having less files is a ridiculous "reason"). The KISS principle applies here too, and from a Spring point of view it would make sense too if you intend to inject the comparators. — Kayaman, Jan 15 '16 at 10:36
Whoever downvoted, please be so kind to explain why this is a silly question in your opinion. Thanks. BTW: True, this is not a real factory, and as I said before, I question the reason "less clutter" myself. — Benjamin Marwell, Jan 15 '16 at 11:08

Sergiy Medvynskyy · Answer 1 · 2016-01-15T10:43:16.383

IMHO the best way is a mix because you have a single entry point to get all your comparators (you don't need to know name of each comparator: all are in the same java class) and you can instantiate all your comparators as a bean.

public class LexicographicComparator implements Comparator<Person> {
    @Override
    public int compare(Person a, Person b) {
        return a.name.compareToIgnoreCase(b.name);
    }
}

// file AgeComparator.java
public class AgeComparator implements Comparator<Person> {
    @Override
    public int compare(Person a, Person b) {
        return a.age < b.age ? -1 : a.age == b.age ? 0 : 1;
    }
}

public class PersonComperatorFactory {

    private static final Comparator<Person> NAME_COMPARATOR = new LexicographicComparator();       
    private static final Comparator<Person> AGE_COMPARATOR = new AgeComparator();

    public static Comparator<Person> getLexicographicComparator() {
        return NAME_COMPARATOR;
    }

    public static Comparator<Person> getAgeComparator() {
        return AGE_COMPARATOR;
    }
}

score 2 · Accepted Answer · answered Jan 15 '16 at 10:41

In my opinion the best way to achieve the desidered result is to have an utility class (putting it in .util package) containing all your static comparators.

Example:

public class Comparators {
    public static final Comparator<Person> PERSON_LEXICOGRAPHIC_COMPARATOR = new Comparator<Person>() {
        @Override
        public int compare(Person a, Person b) {
            return a.name.compareToIgnoreCase(b.name);
        }
    }

    public static final Comparator<Person> PERSON_AGE_COMPARATOR = new Comparator<Person>() {
        @Override
        public int compare(Person a, Person b) {
            return a.age < b.age ? -1 : a.age == b.age ? 0 : 1;
        }
    }
}

Or in the Java 8 way:

public class Comparators {
    public static final Comparator<Person> PERSON_LEXICOGRAPHIC_COMPARATOR = (Person a, Person b) -> a.name.compareToIgnoreCase(b.name);
    public static final Comparator<Person> PERSON_AGE_COMPARATOR = (Person a, Person b) -> (a.age < b.age ? -1 : a.age == b.age ? 0 : 1);
}

I would use directly the static instances in order to do not waste memory and resources, since they do not use any state attribute in the compare method.

This is probably not very friendly for injection, but so far the simplest and/or cleanest solution for API design imho. +1 for the Java8 code. — Benjamin Marwell, Jan 15 '16 at 13:57
@Ben I think that for dependency injection purposes your options are both suitable, but i would reccomend the use of static instances. A good solution for DI is the Sergiy Medvynskyy one, which uses static comparators that are good also if you need non-singleton comparators. If you are sure that your comparators are singleton you could decide to use your singleton non-static comparator instances. — Francesco Pitzalis, Jan 15 '16 at 14:16
I'm making this the accepted answer, because I don't see any advantage to keep comparator in their own classes. Especially not when they are that short (even shorter with Java 8). It might be interesting to keep some very special, long comparators in their own class, though. — Benjamin Marwell, Jul 18 '16 at 06:38

score 2 · Answer 3 · answered Jan 17 '16 at 14:31

The best way to implement an immutable comparator is to implement it as a singleton. According to Effective Java, Item 3, the preferred approach is to use an Enum singleton:

This approach is functionally equivalent to the public field approach, except that it is more concise, provides the serialization machinery for free, and provides an ironclad guarantee against multiple instantiation, even in the face of sophisticated serialization or reflection attacks. While this approach has yet to be widely adopted, a single-element enum type is the best way to implement a singleton.

As for the location of the comparators, API discoverability should be considered. If you implement the comparators in a separate class or even a separate package, the users of your software (i.e. other programmers) might simply overlook them and are tempted to reimplement them (resulting in duplicate code and effort). Therefore, to aid discoverability create static methods that return the comparators either in the Person class itself, or adopt a convention similar to Guava and put them in a utility class named as the plural of the object (e.g. Persons) in the same package. Using this convention allows another programmer to just type Person(s). in their favorite IDE and auto-complete will show all available utility methods.

The complete code looks like this:

public final class Persons {
  private Persons() {}

  public static Comparator<Person> getLexicographicComparator() {
    return Comparators.LEXICOGRAPHIC;
  }

  public static Comparator<Person> getAgeComparator() {
    return Comparators.AGE;
  }

  enum Comparators implements Comparator<Person> {
    LEXICOGRAPHIC {
      @Override
      public int compare(Person a, Person b) {
        return a.name.compareToIgnoreCase(b.name);
      }

      @Override
      public String toString() {
        return Persons.class.getName() + ".getLexicographicComparator()";
      }
    },

    AGE {
      @Override
      public int compare(Person a, Person b) {
        return a.age < b.age ? -1 : a.age == b.age ? 0 : 1;
      }

      @Override
      public String toString() {
        return Persons.class.getName() + ".getAgeComparator()";
      }
    }
  }
}

Note that since it doesn't make sense to instantiate the utility class, the constructor is made private (as per Effective Java, Item 4). Also, to hide the fact that the comparators are actually enums, the enum has package-private (default) accessibility and the toString() methods are overridden to point to their publicly accessible getter method.

score 1 · Answer 4 · answered Jan 16 '16 at 02:17

I would probably implement the Comparators as an enum and I would put the enum either in the same package as Person or else nest it inside the Person class, to provide direct access to the fields which are compared.

public enum PersonComparator implements Comparator<Person> {
    AGE {
        @Override
        public int compare(Person a, Person b) {
            return a.age < b.age ? -1 : a.age == b.age ? 0 : 1;
        }
    },

    NAME {
        @Override
        public int compare(Person a, Person b) {
            return a.name.compareToIgnoreCase(b.name);
        }
    }
}

score 0 · Answer 5 · answered Jan 15 '16 at 10:34

The Factory version is more versatile.

Calling classes don't have to know the real implementation class.

Also, you could add a single entry point, which would decide on which implementation to return, based on a type (here based on a public Enumeration) :

class PersonComperatorFactory {

    public static enum Type {

        AGE, LEXIC
    }

    public static Comparator<Person> getComparator(final Type comparatorType) {

        switch (comparatorType) {

        case AGE:
            return getAgeComparator();
        case LEXIC:
            return getLexicographicComparator();
        default:
            return getLexicographicComparator();

        }

    }

    private static Comparator<Person> getAgeComparator() {
        return new Comparator<Person>() {
            @Override
            public int compare(final Person a, final Person b) {
                return a.age < b.age ? -1 : a.age == b.age ? 0 : 1;
            }
        };
    }

    private static Comparator<Person> getLexicographicComparator() {
        return new Comparator<Person>() {
            @Override
            public int compare(final Person a, final Person b) {
                return a.name.compareToIgnoreCase(b.name);
            }
        };
    }
}

score 0 · Answer 6 · answered Jan 15 '16 at 11:56

A combination will most likely give the best result. Option 1 encapsulates the logic nicely, but accessing these classes directly can produce a lot of clutter. Your 'Factory' does fix this issue, but I disagree with the decision of implementing the logic here.

This requires you to change the Factory, if the logic of the comparators should change, which is not its responsibility.

I recommend keeping your initial classes, and then return an instance of these classes from the factory. Even though you have 3 classes, you will only be working with 1 of them, so the clutter should be manageable.

class PersonComperatorFactory {
    public static Comparator<Person> getAgeComparator() {
        return new LexicographicComparator();
    }

    public static Comparator<Person> getLexicographicComparator() {
        return new AgeComparator();
    }
}

I also propose a third option, which provides zero encapsulation and no reuse, however maximum flexibility. This might not be what you want, just wanted to make sure it was considered.

public class Demo {

    public static void main(String[] args) {
        List<Person> persons = Arrays.asList(new Person(), new Person(), new Person(), new Person());
        SortPersons(persons, (p1, p2) -> p1.age < p2.age ? -1 : p1.age == p2.age ? 0 : 1);
    }

    private static void SortPersons(List<Person> persons, Comparator<Person> comparator){
        //Or what ever logic you need the comparator for
        persons.sort(comparator);
    }

}

Lastly, if your .util package is in any way (or capable of being) deployed separately, I would not throw a dependency of Person into it. In this case, I would put it in .default.

ComperatorFactory using static methods or each in its own class?

6 Answers6