1

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:

  • Online 2013 4
  • Online 2013 1
  • Online 2009 11
  • Online 2008 4
  • Paper 2012 12
  • Paper 2011 9

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:

  • Paper 2012 12
  • Paper 2011 9
  • Online 2013 4
  • Online 2013 1
  • Online 2009 11
  • Online 2008 4

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;
}
4
  • Please show the declarations for the member fields. Commented Jul 8, 2013 at 21:22
  • Is the sort order for type alphabetical or a specialized sort order that you are attempting to define? Commented Jul 8, 2013 at 21:25
  • 2
    Specialized - I want to dictate the order. Commented Jul 8, 2013 at 21:30
  • Thanks for specifying. I'll remove my answer. Commented Jul 8, 2013 at 21:30

3 Answers 3

3

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.

Sign up to request clarification or add additional context in comments.

6 Comments

How would reflectionCompare know that it must compare by types, then by year, then by month?
You are right. I'm not sure, probably it uses the sequence of the fields? I've replaced it with a more robust version, and used an enum for the ArticleType.
Better, but type should come first (as the OP asked), and there's no need to make the enum implement Comparable: that's by default.
So how would I sort if I want to add three more String types (like author and filename)
@user2124871 Just add them to the list of fields in the compareTo method in Article.
|
1

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();
}

3 Comments

Oh, sorry, that was me, but not intentional! Removed it, sorry. Nice example with the ComparisonChain btw!
Unfortunately, I have no control over the datatype of the comparison objects (as they are all Strings).
Then convert them to the appropriate type (enum, int or Integer, int or Integer) before doing the comparison.
0

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

2 Comments

comparing ints by subtracting is not safe with very large values due to overflow. You should prefer Integer.compare(i1, i2) (since Java 7)
@JBNizet In general, you certainly must be careful about subtracting a large positive number and a large negative number. Since we are comparing years here, they are likely not large numbers (on the order of 10^3) nor negative.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.