CODEWITHSUNDEEP
java
user2918640
user2918640

Reputation: 473

Is it a bad practice to create a single comparator for two completely different classes?

I would like to know if it is alright to combine two comparators into a single comparator for two completely different classes. Objects are to be sorted alphabetically by a string type name property which is present in both the classes which is why I created a single comparator.

/**
 * Comparator to sort Book or Magazine alphabetically based on
 * their String type name property.
 */    
public class BookOrMagazineComparator implements Comparator<Object> {

        @Override
        public int compare(Object o1, Object o2) {
            if (o1 != null && o2 != null) {
                if (o1 instanceof Book && o2 instanceof Book) {
                    Book b1 = (Book) o1;
                    Book b2 = (Book) o2;
                    return b1.getName().compareTo(b2.getName());
                } else if (o1 instanceof Magazine && o2 instanceof Magazine) {
                    Magazine m1 = (Magazine) o1;
                    Magazine m2 = (Magazine) o2;
                    return m1.getName().compareTo(m2.getName());
                }
            }
            return 0;
        }

    }

Could there by any side effect if I use the above approach?

Edit: I would like to add that Book/Magazine are actually enums.

Upvotes: 4

Views: 539

Answers (4)

user3437460
user3437460

Reputation: 17454

Yes, it is not advisable to do so. Furthermore, you are only comparing Magazine against Magazine and Book against Book.

If say, you only intend to compare objects from these different classes by their name or a similar attribute, you can make use of the hierarchy and compare by the type of the base class:

class ReadingMaterial
{
    protected String name;
    //other attributes here..
}

class Magazine extends ReadingMaterial
{

}

class Book extends ReadingMaterial
{

}

Now you only need to implement the Comparator once instead of implementing it for all types of classes:

class ReadingMaterialComparator implements Comparator<ReadingMaterial>
{
    //implements comparison here..
}

With the above approach, even if you need to add a new type of ReadingMaterial for example the NewsPaper. You don't even have to edit the Comparator and it will still work. In essence, it makes your implementation more scalable and less coupled.

Upvotes: 0

Jan Zyka
Jan Zyka

Reputation: 17888

It's opinion based but I would call it bad practice as it breaks the rule that every class should serve only one purpose (AKA Single Responsibility principle).

Also it is probably broken because if you pass anything beside Magazine and Book (like InputStream and URL to give wild example) it returns 0 meaning they are equal. You gave up on the type-safety by implementing Comparator<Object> which is shame.

As suggested in the comments it will be good idea to find common interface of the two classes you care about and implement comparator for that interface. In that case it will make perfect sense.

Suggestion: You say the classes are unrelated but they have something in common - the name if you have NamedItem interface which both Magazine and Book implement you can create Comparator<NamedItem> and you are all good to go.

Note: You added the classes are enums. No problem with that. Enum can easily implement interface.

Upvotes: 6

yshavit
yshavit

Reputation: 43391

You're losing a lot of Java's type safety when you do that.

One of the big benefits of having a comparator typed as, say, Comparator<Book> is that if I have a List<DVD>, I can't accidentally try to sort it using that comparator; the compiler won't even let me. But with your approach, my list can be a list of anything: Books, DVDs, Boats, etc. If the list happens to be anything but a list of all Books or a list of all Magazines at run time, I'll probably get surprising behavior.

Note that your comparator breaks transitivity. If I have a Book{Aaa}, a Magazine{Bbb}, and a Book{Ccc}, then:

  • Book{Aaa} == Magazine{Bbb} because of that final return 0
  • Magazine{Bbb} == Book{Ccc} (ditto)
  • Book{Aaa} should == Book{Ccc} by transitivity, but in fact
  • Book{Aaa} < Book{Ccc}

If you fix this (for instance, by throwing an exception at the end of the method instead of returning 0), then the surprising behavior is that the sort will crash unless you have a homogeneous list of Books or Magazines. If you keep the broken contract, then the surprising behavior is undefined, and will probably result in a list that's not correctly sorted (since the sorting algorithm will have made assumptions about what elements it needs to compare, which only hold true if the comparator is transitive).

Upvotes: 1

EJK
EJK

Reputation: 12527

Could there by any side effect if I use the above approach?

Yes. Tight coupling. Your comparator class will have dependencies on each class it references. That will be a slight impediment to re-use, as it will not be able to exist without the classes it references.

As pointed out in another answer - a class should serve only one purpose. That is the single responsibility principle, and this approach is a slight violation of that principle.

Upvotes: 1

Related Questions