Comparator with ArrayList-objects with multiple subclasses

Question

public class WorthComparator implements Comparator<Värdesak> {
    public int compare(Värdesak v1, Värdesak v2) {
        if (v1 instanceof Apparat && v2 instanceof Apparat) {
            return ((Apparat) v1).worth() > ((Apparat) v2).worth() ? 1
                    : ((Apparat) v1).worth() < ((Apparat) v2).worth() ? -1
                            : 0;
        } else if (v1 instanceof Smycke && v2 instanceof Smycke) {
            return ((Smycke) v1).worth() > ((Smycke) v2).worth() ? 1
                    : ((Smycke) v1).worth() < ((Smycke) v2).worth() ? -1
                            : 0;
        } else if (v1 instanceof Aktie && v2 instanceof Aktie) {
            return ((Aktie) v1).worth() > ((Aktie) v2).worth() ? 1
                    : ((Aktie) v1).worth() < ((Aktie) v2).worth() ? -1 : 0;
        }
    }
}

As you can see I'm trying to compare different objects from an ArrayList to then sort them in my GUI by highest value. Each different object ("Smycke", Aktie, Apparat") is in their own respective subclass which each has a method for estimate their value. "Värdesak" is the superclass.

My problem is that I don't know who I will get a return-statement to this or is there's another smart way to do it?

It would be cleaner to make them all implement an interface with `worth()`, or have `worth()` as an abstract method in `Värdesak`. Then you could call `worth()` without any casts or `instanceof` checks. — kiheru, Apr 02 '15 at 10:10
Note: perhaps one better modifies the `Värdesak` into `Vardesak`. [using unicode is generally not adviseable](http://stackoverflow.com/questions/2793792/is-it-a-good-idea-to-use-unicode-symbols-as-java-identifiers). — Willem Van Onsem, Apr 02 '15 at 10:25
@CommuSoft That may be generally advisable, but once you expect readers to make sense of identifiers in Swedish, misspelling the words hardly makes them more readable. — kiheru, Apr 02 '15 at 10:50

score 1 · Answer 1 · edited May 23 '17 at 11:57

Looks to me the design of your code is wrong, or you applying it the wrong way. Since every "Värdesak" is worth something, you can/need to define this method at the Värdesak level:

public abstract class Värdesak {

    public abstract double worth ();

}

And then:

public class Apparat extends Värdesak {

    @Override
    public double worth () {
        return 500.0d;
    }

}

Or if the price is fixed, you can use a field:

public abstract class Värdesak {

    private double value;

    public double worth () {
        return worth;
    }

}

Now you can simply use the dynamic binding principle:

public class WorthComparator implements Comparator<Värdesak> {

    public int compare(Värdesak v1, Värdesak v2) {
        return Double.compare(v1.worth(),v2.worth());
    }

}

instanceof calls are in general considered to be a bad smell: if you have to use them, there is normally something wrong with the design of your code.

Another advice, please don't use unicode characters in identifiers: use Vardesak instead of Värdesak.

score 1 · Answer 2 · answered Apr 02 '15 at 22:37

1

Thank you for all the good critique. Really appriciate it! I know about the Unicode, and it will defenetely be fixed when I clean the code. Do you mean that a "general" method in the superclass (worth()) that you override would fix my problem? Thank you

answered Apr 02 '15 at 22:37

Joel Larsson

23
3

you should post this as a comment, otherwise people will start downvoting... And yes, a general method will probably solve the problem. From the moment you have to work with instance of, it breaks object-oriented design. – Willem Van Onsem Apr 02 '15 at 22:41
1

Thank you for informing me. I'm very much a novice being on StackOverflow. Ok, I'll try my best, thank you – Joel Larsson Apr 03 '15 at 09:30

alex · Answer 3 · 2015-04-02T12:03:30.620

0

You could also let Värdesak implement Comparable interface

public abstract class Värdesak implements Comparable<Värdesak> {
    //[...] your code
}

Now you define compareTo for every class extends Värdesak

public class Smycke extends Värdesak {

    //[...] your code

    @Override
    public int compareTo(Server o) {
        // TODO: your compare logic
        return 0;
    }
}

Later you can sort your Värdesaks in this way: Collections.sort(värdesakList)

EDIT: Note, this solution is not so flexible like Comparator pattern.

edited Apr 02 '15 at 12:03

answered Apr 02 '15 at 11:40

alex

8,904
6
49
75

One in general implements a `Comparator` if there are multiple ways to order a list of items. For instance by name, by value, by id-number,... – Willem Van Onsem Apr 02 '15 at 11:48

Comparator with ArrayList-objects with multiple subclasses

3 Answers3