Sort by year and day descending, yet time ascending

Question

I recently discovered a hugely inefficient portion of code in my program, and have gone about rewriting it. I am however, not quite sure if the rewritten program actually does exactly what I want, due to its (relative) mathematical complexity. I am especially uncertain about how this new code will handle changes of the day (so 24:00 -> 00:00).

Here are the old and the new piece of code, in said order:

Collections.sort(listEntries, new Comparator<LogEntry>() {
            @Override
            public int compare(LogEntry lhs, LogEntry rhs) {
                Calendar lhsCreatedAt = Calendar.getInstance();
                Calendar rhsCreatedAt = Calendar.getInstance();
                lhsCreatedAt.setTimeInMillis(lhs.getCreatedAt());
                rhsCreatedAt.setTimeInMillis(rhs.getCreatedAt());
                if (lhsCreatedAt.get(Calendar.YEAR) != rhsCreatedAt.get(Calendar.YEAR)) {
                    return rhsCreatedAt.get(Calendar.YEAR) - lhsCreatedAt.get(Calendar.YEAR);
                }
                if (lhsCreatedAt.get(Calendar.DAY_OF_YEAR) != rhsCreatedAt.get(Calendar.DAY_OF_YEAR)) {
                    return rhsCreatedAt.get(Calendar.DAY_OF_YEAR) - lhsCreatedAt.get(Calendar.DAY_OF_YEAR);
                }
                return (int) (lhs.getCreatedAt() - rhs.getCreatedAt());
            }
        });

versus

Collections.sort(listEntries, new Comparator<LogEntry>() {
            @Override
            public int compare(LogEntry lhs, LogEntry rhs) {
                long lhsCreatedAt = lhs.getCreatedAt();
                long rhsCreatedAt = rhs.getCreatedAt();
                long millisecondsPerDay = 86400000;

                if (Math.floor(lhsCreatedAt/millisecondsPerDay) != Math.floor(rhsCreatedAt/millisecondsPerDay)){
                    return (int) (rhsCreatedAt - lhsCreatedAt);
                }
                return (int) (lhsCreatedAt - rhsCreatedAt);
            }
        });

If there are any flaws in this new code, or any other performance increases to be gained, I would love to know.

Answer 1

Main problem that I see, is cast to int, as it will overflow in 23 days!!!

Regarding 24:00 correctness, that depends is that ticks in local timezone or UTC and if you care, but I'd suggest to write Unit Test that validates this algorithm on some set of events, for whole year, and lot of events per day, to verify every case.

Additionally make millisecondsPerDay a constant inside Comparator, but upper then method

new Comparator<LogEntry>() {
        static final long MILLISECONDS_PER_DAY = 86400000;
        @Override
        public int compare(LogEntry lhs, LogEntry rhs) { ...}
}

EDIT

You don't need Math.floor, as this is integral numbers, so dividing gives number of days as integral number.

I'd suggest following code:

Collections.sort(listEntries, new Comparator<LogEntry>() {

        static final long MILLISECONDS_PER_DAY = 86400000;

        @Override
        public int compare(LogEntry lhs, LogEntry rhs) {
            long lhsCreatedAt = lhs.getCreatedAt();
            long rhsCreatedAt = rhs.getCreatedAt();

            long lhsDays = lhsCreatedAt/MILLISECONDS_PER_DAY;
            long rhsDays = rhsCreatedAt/MILLISECONDS_PER_DAY;
            if(lhsDays == rhsDays){
                 return (int)(lhsCreatedAt%MILLISECONDS_PER_DAY - rhsCreatedAt%MILLISECONDS_PER_DAY)
            }
            return (int)(rhsDays-lhsDays);
        }
    });

Well, if block can be following, as long as we are in the same day, no need to divide additional time:

            if(lhsDays == rhsDays){
                 return (int)(lhsCreatedAt - rhsCreatedAt)
            }