Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

In my current project, I have to deal a lot with dates, especially checking if the date entered by user is today, or max one year from today etc. As I find the datetime API of Java7 (which I regrettably still have to use) pretty confusing, in order to keep my code clean and reada, I wrote the folowing method:

public static int compareDates(Date date1, Date date2) {
    SimpleDateFormat math = new SimpleDateFormat("yyyyMMdd"); // I'm working with servlets, so keeping it thread-safe
    Long date1asLong = new Long(math.format(date1));
    Long date2asLong = new Long(math.format(date2));
    return date1asLong.compareTo(date2asLong);
}

// usage, e.g. to find if the date entered by user is greater than today
if (MyDateUtils.compareDates(dateFromUser, new Date()) > 0) {
    // do the work
} else {
    // scream
}

Anyway, I am not sure whether this approach does not havy some serious drawbacks, so I am throwing it here for review. Looking forward to your comments.

share|improve this question
    
please check also this answer (and comments below) – chillworld Jan 7 at 12:33
1  
Have you looked at using the Joda Time library? joda.org/joda-time – Freiheit Jan 7 at 16:05
    
Do you have any libraries on the classpath? Apache Commons stuff? – Boris the Spider Jan 7 at 17:42

It's an interesting, simple approach.

But you don't need new Long objects. Better use primitive types instead.

In fact, as @njzk2 pointed out in a comment, you don't need long at all, as dates in yyyyMMdd format are alphabetically sorted, you can use simple String comparison.

It's good you mention in comments that you create a SimpleDateFormat instance inside the method for thread safety. Another common solution in such situations is to use a ThreadLocal. You can wrap a SimpleDateFormat instance in a ThreadLocal so that all threads will have their own copy, eliminating thread safety issues.

Last but not least, "math" is a poor name for a date formatter object.

Putting my suggestions together:

private static final ThreadLocal<DateFormat> dateFormatThreadLocal = new ThreadLocal<DateFormat>() {
    @Override
    protected DateFormat initialValue() {
        return new SimpleDateFormat("yyyyMMdd");
    }
};

public static int compareDates(Date date1, Date date2) {
    DateFormat dateFormat = dateFormatThreadLocal.get();
    return dateFormat.format(date1).compareTo(dateFormat.format(date2));
}
share|improve this answer
    
Bear in mind that the TheadLocal approach will leak memory if the application undeployed. A private static final BlockingQueue would be better... – Boris the Spider Jan 7 at 17:40
2  
you don't even need the long at all. this format sorts dates by alphabetical order. – njzk2 Jan 7 at 18:23
1  
also, what thread safety issue? I don't think a call to format needs to be thread safe. – njzk2 Jan 7 at 18:24
    
Thanks @njzk2, good point about the alphabetic ordering, I incorporated that in my answer. As for thread safety, OP wrote that the class is used by a servlet, and may need to be invoked from multiple threads, so format need to be made thread safe. – janos Jan 7 at 18:52
    
@janos after taking a look at openjdk's implementation for format, it appears you are right! format does modify and depends on the internal state of the SimpleDateFormat object (namely it changes the date on a local Calendar object) – njzk2 Jan 7 at 19:15

You may simply truncate given date to days (ignoring time part), one possible way to do it:

date1.toInstant().truncate(ChronoUnit.DAYS).isAfter(date2.toInstant())

It's faster than performing that conversions and IMO easier to read because methods are self-descriptive (truncated to days date is after...). You may want to have an helper method for that:

public static boolean isAfterToday(Date date) {
    return date.toInstant(truncate(ChronoUnits.DAYS).isAfter(Instant.now());
}

Used like this:

if (MyDateUtils.isAfterToday(dateFromUser)) {
    // do the work
} else {
    // scream
}

Note that if you heavily use dates you may also want to use Joda Time, things will be even simpler:

public static boolean isAfterToday(DateTime date) {
    return DateTimeComparator.getDateOnlyInstance().compare(date, DateTime.now()) > 0;
}

If you can't use Instant (and obviously you don't want to include Joda Time just for this) you should at least use Calendar as described here, in short:

public static long getDatePart(Date date) {
    Calendar cal = Calendar.getInstance();
    cal.setTime(date);
    cal.set(Calendar.HOUR_OF_DAY, 0);
    cal.set(Calendar.MINUTE, 0);
    cal.set(Calendar.SECOND, 0);
    cal.set(Calendar.MILLISECOND, 0);

    return cal.getTimeInMillis();
}

Used as:

if (getDatePart(dateFromuser) > getDatePart(Date.now())) {
}
share|improve this answer
    
Thanks. Regrettably, there is no such thing as toInstant() in Java7, to wich I am restricted due to customer's environment. The use of JodaTime seems to be a good idea, although it is still a bit to chatty. – Jozef Chocholacek Jan 7 at 12:03
    
Then I'd go with linked approach using Calendar. It's faster and reusable (you may want to extract date part in some other circumstances). – Adriano Repetti Jan 7 at 12:05
    
Note that Instant.truncatedTo() will truncate using UTC offset. – bowmore Jan 8 at 16:37

Main remarks

  1. Use the existing Comparator abstraction

    If you're solving a problem for comparisons, make a Comparator. Instead of having a method that compares Date instances on some util class, have a method there that returns a Comparator instead. Or even, make a standalone Comparator class. In this case, don't forget to document that it's not consistent with equals().

  2. Don't forget about time zones

    If you want to do calendar operations on a Date (and yes comparing by day is just that), then make sure you think about time zones. As it is, your code just takes the server's default time zone (which may be fine for your use case). It's just better to make this explicit, which also makes this more reusable and easier to test.

    Why is TimeZone important for calendar operations? Well since a single Date instance can fall on different calendar days depending on the TimeZone. If you have users in different time zones, they'll expect their entered dates to be interpreted vs. the time zone they're in.

  3. Use the right tool for the job: java.util.Calendar

    Using SimpleDateFormat is clever, but it's just not the tool for the job. Use Calendar. The SimpleDateFormat class will be making a Calendar under the hood anyway, it will just also have to figure out formating it, and then after all the formatting, you'll just convert it to a number, while Calendar already has the numbers you need to compare.

Minor remarks

  1. String is Comparable too

    Your conversion from String to Long is unnecessary, you could have compared the Strings directly. The format you use yields Strings that have the same ordering as the respective Long instances.

  2. Use primitives whenever possible

    If you did want to use numbers then prefer primitives if possible. Long.parseLong() and Long.compare() could help you avoid the boxes.

Example code

public static Comparator<Date> byDayComparator(final TimeZone timeZone) {
    return new Comparator<Date>() {
        @Override
        public int compare(Date date1, Date date2) {
            return truncateToDay(date1).compareTo(truncateToDay(date2));
        }

        private Date truncateToDay(Date date) {
            Calendar calendar = Calendar.getInstance(timeZone);
            calendar.setTime(date);
            calendar.set(Calendar.HOUR_OF_DAY, 0);
            calendar.set(Calendar.MINUTE, 0);
            calendar.set(Calendar.SECOND, 0);
            calendar.set(Calendar.MILLISECOND, 0);
            return calendar.getTime();
        }
    };
}
share|improve this answer

This isn't as transparent as the calendar version, but you can easily do this by dividing the time value to get a number of days. E.g. long one-day = 24*60*60*1000 int compare = Long.compare(d1.getTime()/oneDay,d2.getTime()/oneDay)

share|improve this answer
3  
This will fail because of DST. – Artefacto Jan 7 at 17:01

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.