Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

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

Tags:

java

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.

like image 873
user2918640 Avatar asked Feb 07 '23 14:02

user2918640


2 Answers

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.

like image 78
Jan Zyka Avatar answered Feb 14 '23 00:02

Jan Zyka


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.

like image 32
EJK Avatar answered Feb 13 '23 23:02

EJK