CODEWITHSUNDEEP
javacompareto
user2124871
user2124871

Reputation: 813

Sorting objects using compareTo (multiple types)

So first, I have an object called news article that has three properties that I need to sort by:

Year (int), Month (int), type (String - online, paper)

So for example, it would be like this:

What is going on is that the month and year appear to be sorting correctly, but I'm having problems sorting by type. What's the proper way to sort by String in a compareTo?

Results at the moment:

Here's my method (I apologize for it being somewhat quirky, I've been trying different ways to sort by the type and was experimenting):

@Override
public int compareTo(Article rs) {

    Integer x = 0;


        Integer year1 = 0;
        Integer year2 = 0;
        Integer month1 = 99999;
        Integer month2 = 99999;
        Integer type1 = 99999;
        Integer type2 = 99999;

        if(rs.year != null && year != null) {

            if(!rs.year.equals(""))
                year1 = Integer.parseInt(rs.year);
            if(!year.equals(""))
                year2 = Integer.parseInt(year);
        }

        if(rs.month != null && month != null) {
            if(!rs.month.equals(""))
                month1 = Integer.parseInt(rs.month);
            if(!month.equals(""))
                month2 = Integer.parseInt(month);
        }

        if(rs.type == null)
            type1 = 99999;
        else
            type1 = 0;

        if(type == null)
            type2 = 99999;
        else
            type2 = 0;

        x = type2.compareTo(type1);
        if(x != 0) {
            return x;
        }

        x = year1.compareTo(year2);
        if(x != 0) {
            return x;
        }

        x = month1.compareTo(month2);
        if(x != 0) {
            return x;
        }


    return x;
}

Upvotes: 1

Views: 1228

Answers (3)

Code-Apprentice
Code-Apprentice

Reputation: 83517

To answer your question, you can simply use compareTo() to compare two String objects. You just have to make sure that you compare them in the order that you want. Doing

x = type2.compareTo(type1);

compares the Strings in descending order. If you want to sort in ascending order, you need to do

x = type1.compareTo(type2);

In adition, I am curious about all the == null or != null checks. Since this is your own class, you should control whether or not the member fields can be null. If I were writing a similar class, I would require that all fields are initialized by the constructor so that I don't need to check for null values. This can greatly simplify all the other methods in the class, including this compareTo() method.

Also, you should prefer built-in types over wrapper classes. In other words, use ints for the year and month fields rather than Integers. This can also help simplify your code in a couple of ways. First, you won't have to worry about null values. Second, you can compare to ints with a simple subtraction:

int compareYears = this.year - rs.year;
if (compareYears != 0) {
    return compareYears;
}

With these suggestions, you can not only fix the current problem, but you can also reduce the number of lines of code by at least half.

(Note that in general, you must be careful about comparing ints using subtraction due to overflow. In this case, since we are comparing years, neither value should be negative, so there shouldn't be a problem. Of course, the rest of your class should enforce that the year field is in fact a valid year.)

Upvotes: 0

JB Nizet
JB Nizet

Reputation: 691625

If the type is not null, you replace it by 0, whatever its value is. So comparing "paper" with "online" leads to comparing 0 with 0, which is obviously wrong.

My first advice would be to use proper types instead of String for everything. month and year should be ints, and type should be an enum. You should also strive to make them non-nullable.

Once done, the comparison method would reduce to

public int compareTo(Article other) {
    int result = this.type.compareTo(other.type);
    if (result == 0) {
        result = Integer.compare(this.year, other.year);
    }
    if (result == 0) {
        result = Integer.compare(this.month, other.month);
    }
    return result;
}

Note that using an enum allows you to dictate specify the way types compare, simply by listing the values in the order you want.

Or even better, with Guava:

public int compareTo(Article other) {
    return ComparisonChain.start()
                          .compare(this.type, other.type)
                          .compare(this.year, other.year)
                          .compare(this.month, other.month)
                          .result();
}

If the values are nullable, the above code would have to be changed to

public int compareTo(Article other) {
    return ComparisonChain.start()
                .compare(this.type, other.type, Ordering.natural().nullsLast())
                .compare(this.year, other.year, Ordering.natural().nullsLast())
                .compare(this.month, other.month, Ordering.natural().nullsLast())
                .result();
}

Upvotes: 1

Erik Pragt
Erik Pragt

Reputation: 14617

I would refactor (eg throw away) your complete code and replace it by using CompareToBuilder.

This will create the following code:

enum ArticleType {
    ONLINE, PAPER
}

class Article implements Comparable<Article> {

    int year;
    int month;
    ArticleType type;

    Article(int year, int month, ArticleType type) {
        this.year = year;
        this.type = type;
        this.month = month;
    }

    @Override
    public int compareTo(Article o) {
        return new CompareToBuilder()
                .append(this.type, o.type)
                .append(this.year, o.year)
                .append(this.month, o.month)
                .toComparison();

    }

    @Override
    public String toString() {
        return Objects.toStringHelper(this)
                .add("month", month)
                .add("year", year)
                .add("type", type)
                .toString();
    }
}

@Test
public void testSortArticles() throws Exception {
    List<Article> articleList = new ArrayList<>();
    articleList.add(new Article(2012, 1, ArticleType.ONLINE));
    articleList.add(new Article(2011, 1, ArticleType.ONLINE));
    articleList.add(new Article(2011, 6, ArticleType.ONLINE));
    articleList.add(new Article(2010, 1, ArticleType.ONLINE));
    articleList.add(new Article(2010, 1, ArticleType.PAPER));
    articleList.add(new Article(2010, 2, ArticleType.PAPER));
    articleList.add(new Article(2010, 3, ArticleType.PAPER));
    articleList.add(new Article(2012, 1, ArticleType.PAPER));
    articleList.add(new Article(2012, 9, ArticleType.PAPER));

    Collections.sort(articleList);

    System.out.println(articleList);
}

Printing this will lead to the following:

[Article{month=1, year=2010, type=ONLINE}, Article{month=1, year=2010, type=PAPER},
 Article{month=2, year=2010, type=PAPER}, Article{month=3, year=2010, type=PAPER},
 Article{month=1, year=2011, type=ONLINE}, Article{month=6, year=2011, type=ONLINE},
 Article{month=1, year=2012, type=ONLINE}, Article{month=1, year=2012, type=PAPER},
 Article{month=9, year=2012, type=PAPER}]

Will provides a nicely sorted list. The offline/online is sorted too using an Enum (ArticleType). In my opinion, this looks a bit better than your current implementation.

Upvotes: 3

Related Questions