How many problems are in the following date parsing routines that come from a real-world project?

Question

These methods are laughably stupid, IMO, but I want to get a feel for what other developers think of such code. Criticisms may include technical and stylistic errors. Corrections may use anything from Apache commons-lang, such as StringUtils, DateUtils, etc, as well as anything in Java 5. The code is intended for a web application, if that would affect your style. These four methods are all defined in the same file, too, if that matters. Did I mention that there are no unit tests for this code either?! What would you do to fix the situation? I just happened upon this file, and it's not my immediate task to fix this code. I could in my spare time, if so desired.

Method one:

   public static boolean isFromDateBeforeOrSameAsToDate(final String fromDate,
     final String toDate) {
 boolean isFromDateBeforeOrSameAsToDate = false;
 Date fromDt = null;
 Date toDt = null;
 try {
     fromDt = CoreUtils.parseTime(fromDate, CoreConstants.DATE_PARSER);
     toDt = CoreUtils.parseTime(toDate, CoreConstants.DATE_PARSER);
     // if the FROM date is same as the TO date - its OK
     // if the FROM date is before the TO date - its OK
     if (fromDt.before(toDt) || fromDt.equals(toDt)) {
  isFromDateBeforeOrSameAsToDate = true;

     }
 } catch (ParseException e) {
     e.printStackTrace();
 }
 return isFromDateBeforeOrSameAsToDate;
    }

Method two:

    public static boolean isDateSameAsToday(final Date date) {
 boolean isSameAsToday = false;

 if (date != null) {
     Calendar current = Calendar.getInstance();
     Calendar compare = Calendar.getInstance();
     compare.setTime(date);

     if ((current.get(Calendar.DATE) == compare.get(Calendar.DATE))
      && (current.get(Calendar.MONTH) == compare
       .get(Calendar.MONTH))
      && (current.get(Calendar.YEAR) == compare
       .get(Calendar.YEAR))) {
  isSameAsToday = true;
     }

 }
 return isSameAsToday;
    }

Method three:

    public static boolean areDatesSame(final String fromDate,
     final String toDate) {
 boolean areDatesSame = false;
 Date fromDt = null;
 Date toDt = null;
 try {
     if (fromDate.length() > 0) {
  fromDt = CoreUtils.parseTime(fromDate,
   CoreConstants.DATE_PARSER);
     }
     if (toDate.length() > 0) {
  toDt = CoreUtils.parseTime(toDate, CoreConstants.DATE_PARSER);
     }
     if (fromDt != null && toDt != null) {
  if (fromDt.equals(toDt)) {
      areDatesSame = true;
  }
     }

 } catch (ParseException e) {
     if (logger.isDebugEnabled()) {
  e.printStackTrace();
     }
 }
 return areDatesSame;
    }

Method four:

    public static boolean isDateCurrentOrInThePast(final Date compareDate) {
 boolean isDateCurrentOrInThePast = false;
 if (compareDate != null) {
     Calendar current = Calendar.getInstance();
     Calendar compare = Calendar.getInstance();
     compare.setTime(compareDate);

     if (current.get(Calendar.YEAR) > compare.get(Calendar.YEAR)) {
  isDateCurrentOrInThePast = true;
     }

     if (current.get(Calendar.YEAR) == compare.get(Calendar.YEAR)) {
  if (current.get(Calendar.MONTH) > compare.get(Calendar.MONTH)) {
      isDateCurrentOrInThePast = true;
  }

     }

     if (current.get(Calendar.YEAR) == compare.get(Calendar.YEAR)) {
  if (current.get(Calendar.MONTH) == compare.get(Calendar.MONTH)) {
      if (current.get(Calendar.DATE) >= compare
       .get(Calendar.DATE)) {
   isDateCurrentOrInThePast = true;
      }

  }

     }

 }
 return isDateCurrentOrInThePast;
    }

Here is how I would tend to write the same thing (well, first I would write unit tests, but I'll skip that here).

    public static int compareDatesByField(final Date firstDate,
     final Date secondDate, final int field) {

 return DateUtils.truncate(firstDate, field).compareTo(
  DateUtils.truncate(secondDate, field));
    }

    public static int compareDatesByDate(final Date firstDate,
     final Date secondDate) {
 return compareDatesByField(firstDate, secondDate, Calendar.DATE);
    }

// etc. as required, although I prefer not bloating classes which little
// methods that add little value ...

// e.g., the following methods are of dubious value, depending on taste
    public static boolean lessThan(int compareToResult) {
 return compareToResut < 0;
    }
    public static boolean equalTo(int compareToResult) {
 return compareToResut == 0;
    }
    public static boolean greaterThan(int compareToResult) {
 return compareToResut > 0;
    }
    public static boolean lessThanOrEqualTo(int compareToResult) {
 return compareToResut <= 0;
    }
    public static boolean greaterThanOrEqualTo(int compareToResult) {
 return compareToResut >= 0;
    }

// time-semantic versions of the dubious methods - perhaps these go in TimeUtils ?


   public static boolean before(int compareToResult) {
 return compareToResut < 0;
    }
    public static boolean on(int compareToResult) {
 return compareToResut == 0;
    }
    public static boolean after(int compareToResult) {
 return compareToResut > 0;
    }
    public static boolean onOrBefore(int compareToResult) {
 return compareToResut <= 0;
    }
    public static boolean onOrAfter(int compareToResult) {
 return compareToResut >= 0;
    }

Clients could then use the method as follows:

/* note: Validate library from Apache Commons-Lang throws 
 * IllegalArgumentException when arguments are not valid 
 * (this comment would not accompany actual code since the
 * Javadoc for Validate would explain that for those unfamiliar with it)
 */
 Validate.isTrue(onOrAfter(compareDatesByDate(registrationDate, desiredEventDate),
     "desiredEventDate must be on or after the *day* of registration: ", desiredEventDate);

So one technical error that may not be obvious from the sample code posted above is that it uses a static DateFormat instance to parse String dates! DateFormat is not thread-safe without proper synchronization so the code is *wrong* in an objective sense, as well as being stylistically lame! — les2, Dec 18 '09 at 16:08
The exception handling is apalling! We use log4j, so a if(logger.isDebugEnabled()) { logger.debug("could not parse date '" + dateString + "'"); } would be better than e.printStackTrace();, if logging is needed at all - a simple "// swallow stack trace since we ignore parse errors by returning null or false c" comment in the catch block might be sufficient! — les2, Dec 18 '09 at 16:10
LES2 - without seeing how they are using the static DateFormat, it is hard to say if this is not thread safe. The operation of parsing a string into a date should not effect the internal state of any DateFormat object. If, however, they are adjusting the Calendar associated with the DataFormat, then there is a problem. Why Sun made DateFormat mutable on these few parameters is just beyond me. Anyway, for the way that most folks use DateFormat, I'm pretty sure that it is safe to have a static instance. — Kevin Day, Dec 19 '09 at 05:15
Kevin : that was useful information. I should point out (I'm sure you know this) that the javadoc for DateFormat says "Date formats are not synchronized. It is recommended to create separate format instances for each thread. If multiple threads access a format concurrently, it must be synchronized externally.". The DateFormat objects are used all over the code base, and there's no guarantee that an inexperienced dev won't do something incorrect with it. Doesn't 'defensive programming' demand that you not let people ef stuff up? The problem would be nearly impossible to debug should one occur. — les2, Dec 21 '09 at 17:22

score 0 · Accepted Answer · answered Dec 18 '09 at 16:05

0

First thing, fix the indentation. Ctrl+Shift+F in Eclipse.

I can't stand incorrectly indented code.

Next, write unit tests for all the methods you touch, before you touch them.

Also, use JodaTime. It beats the standard Java date classes to a bloody pulp. A lot of the ugly date logic will be taken care of by switching to it.

answered Dec 18 '09 at 16:05

Ben S

68,394
30
171
212

I would love to use JodaTime, but it would need to be approved by an "architecture approval process" that is ridiculously bureaucratic. In addition, a release is eminent and the required artifacts have already been submitted for approval. There's no way JodaTime could be used (unless already on the list of approved technologies, and I doubt that it is). Did I mention that I'm not a tech lead on this project? – les2 Dec 18 '09 at 16:17
P.S. The code is correctly formatted in the source code base to the Java standard as defined in Eclipse formatting tool. That is one of the things I contributed to the code when we got started with this version - project-wide Eclipse 'Save Actions' that 1) automatically format code and 2) cleans up the code - organize imports, remove unnecessary casts, etc., whenever a file is saved! Stack overflow didn't like the Java standard formatting (which uses a mixture of tabs and spaces for indentation) when pasted. :( – les2 Dec 18 '09 at 16:20
I don't understand why you're asking for suggestions if you're not capable of actually implementing meaningful changes. – Ben S Dec 18 '09 at 16:33
from the post: 1) I want to get a feel for what other developers think of such code. 2) What would you do to fix the situation? Perhaps I would like to learn what to do if I had the power to instantly fix it. Perhaps I would like to get a quick sanity check on my personal feelings about the code - it's possible that stackoverflow could have said, "hey, that code is awesome - it's you that's stupid." And 'switching to JodaTime' is not the only solution! I can (and have in the past for other situations) write unit tests to cover the code. – les2 Dec 18 '09 at 16:49

score 0 · Answer 2 · answered Dec 18 '09 at 16:07

0

There are all sort of issues here. For instance: why static method? But my #1 problem is Lack of unit tests. Whatever refactoring we'd like to apply here, we need tests to make sure we didn't break anything.

Thus, I will start with writing unit tests. All other issues are secondary to that.

answered Dec 18 '09 at 16:07

Itay Maman

30,277
10
88
118

It's a static utility class that's used by various components - e.g., struts actions and forms, Spring services, other classes. It's similar to the methods in StringUtils or DateUtils (from Apache Commons-Lang). You should not instantiate the class containing these utility methods! – les2 Dec 18 '09 at 16:25

How many problems are in the following date parsing routines that come from a real-world project?

2 Answers2