4

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.

user2918640
  • 473
  • 1
  • 7
  • 25
  • 5
    It would make more sense to define a common interface for the two classes that would have the `getName` method. This would allow you to implement `Comparator`. – Eran Mar 03 '16 at 06:59
  • You can inherit these classes from some base class and make single generic comparator for both – Andrey Saleba Mar 03 '16 at 06:59
  • 1
    @Eran, that wouldn't technically offer the same functionality, since we're considering objects of different types to be equivalent. Not that that makes a lot of sense. – shmosel Mar 03 '16 at 07:03
  • @shmosel I don't think that was the OP's intention. It's probably a bug in the code posted in the question. – Eran Mar 03 '16 at 07:04
  • @Eran you're likely correct. Unless OP knows it won't be used to compare Books to Magazines. That would be depressing. – shmosel Mar 03 '16 at 07:06
  • All I wanted is some re-usability here which I realized would not work well since it breaks lots of laws of good practices already. I understand that making a generic comparator which accepts `Object` is an ignorance (or say acceptance) for all types. Not a good idea. And I never intended to compare Books with Magazines :P – user2918640 Mar 03 '16 at 08:17

4 Answers4

6

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.

Community
  • 1
  • 1
Jan Zyka
  • 17,460
  • 16
  • 70
  • 118
  • Appreciate your inputs. Since API is third party source and it wouldn't let me create a common interface for enums, I'd rather go ahead with creating a strongly typed comparator for each type which is anyway a right thing to do from all angles and gets checked at compilation phase itself. – user2918640 Mar 03 '16 at 07:19
1

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.

EJK
  • 12,332
  • 3
  • 38
  • 55
1

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).

yshavit
  • 42,327
  • 7
  • 87
  • 124
  • I am happy to know that in case of a well typed comparator, compiler itself won't accept anything else. That make things clear. – user2918640 Mar 03 '16 at 07:15
0

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.

user3437460
  • 17,253
  • 15
  • 58
  • 106