views:

139

answers:

8

I am reviewing some code at work and came across an inconsistency in how the code handles adding 1 week to the current time and was wondering if there was any reason why one should really be preferred over the other:

The first was a utility method:

public static Date addDaysToDate(final Date date, int noOfDays) {
    Date newDate = new Date(date.getTime());

    GregorianCalendar calendar = new GregorianCalendar();
    calendar.setTime(newDate);
    calendar.add(Calendar.DATE, noOfDays);
    newDate.setTime(calendar.getTime().getTime());

    return newDate;
}

And the second used simple millisecond arithmetic:

long theFuture = System.currentTimeMillis() + (86400 * 7 * 1000);
Date nextWeek = new Date(theFuture);

The second method obviously uses 'magic numbers' to define a week, but this could be moved to a constant MILLISECONDS_IN_ONE_WEEK = 86400 * 7 * 1000 So other than that, is there any reasons why one of these methods should be preferred over the other?

Basically I want to change the code to be consistent throughout, but I'm not entirely sure which one to remove. So any arguments one way or the other would be useful.

A: 

The more general, the more useful it will be. If you always add a week, I'd chose the second method. If you have different adds, I'd chose the first one, perhaps replacing days for seconds or even millis if needed.

Jaime Pardos
+1  A: 

The first will be slower, so if performance is an issue, the second one is better.

Rich
It is run on a Windows Mobile device, so performance is definitely a concern. How much of an impact do you think it would have (roughly)?
DaveJohnston
Depends how often it's called, that kind of thing. To have an idea you'd need to profile it. As an example though, in some time critical code I had to look over, we were constantly calling "new Date().getTime()" - replacing that with "System.currentTimeMillis()" made a significant difference because we weren't repeatedly instantiating Date objects.
Rich
"will be slower" is pretty subjective. Calendar and date field manipulation is mostly about changing the underlying `long` representation of the date anyway, which means that "slower" comes down to how fast it is to create objects - which is much much faster than most people think.
matt b
I agree, I clarified in my reply above.
Rich
+1  A: 

First and foremost, I would argue that you replace it with JodaTime. http://joda-time.sourceforge.net/ It is a very nice time library. You'll want to look at this page to see how easy it is to add days or weeks to a particular point in time: http://joda-time.sourceforge.net/key_period.html Can't do this, mobile device with incompatible JVM. Bummer.

Your first example is easier to read and will be easier to use by your developers. It also uses the Calendar classes which is the generally accepted way to manipulate dates in Java. What makes it better is that it has a clear method name that sets the expectation for what it does.

So if you refactor your system to consistently use com.DaveJ.util.date.DateUtils.addDaysToDate(final Date date, int noOfDays) you can then do whatever you want inside that method, be it Calendar or millis or Joda, and be consistent within your application. Don't forget to write some unit tests for it!

Freiheit
Another answer mentioned speed. If you have this logic encapsulated in one method, in one place, it will be VERY easy to benchmark it and tune it for speed as necessary.
Freiheit
A: 

It depends! Because of leap seconds, DST and other calendar oddities, the two are not always equivalent.

For business and every day use, always use the first method and the performance are not bad at all. It will handle those things for you.

For scientific needs, often you have to use a monotone clock (here the second one).

Po' Lazarus
A: 

The two methods might give different results when a change in daylight saving time is involved. Imagine the current time is 23:50 and at 02:00 the clock jumps to 03:00. When you just add 7 days in milliseconds the time would be 00:50 on the following day. Addings 7 days, the resulting time would still be 23:50.

To make the confusion complete, you could also try add(Calendar.WEEK_OF_YEAR, 1), not sure how that would differ though.

Jörn Horstmann
+9  A: 

The two methods will behave differently on daylight savings boundaries. The first method will continue returning the same time of the day, regardless of daylight savings status. The second method will return times which vary an hour in each direction as daylight savings time starts and stops.

Erick Robertson
A: 

Since you're using it on the mobile device, the first method is preferable. The reason is that your code should be independent of specific calendar, DST and other problems, such as seconds overrun(leap seconds).

You have to remove dependence on GregorianCalendar and create it using Calendar.getInstance().

eugener
You really _shouldn't_ use `getInstance()`, in this case. I'm guessing the code is trying to calculate 1 week in the future according to the Gregorian Calendar, not one week in the future according to the user's local calendar. Although why the user would be using the Decimal Calendar from the French Revolution, I have no idea.
Chris B.
A: 

Be very careful about using method two which caught me out the other day. Consider

private static long ONE_YEAR_AS_MILLISECONDS = 365*24*60*60*1000;

This looks innocent enough, but in fact will not produce what is expected as the multiplication uses integers which, when multiplied by each the other integers, causes a numeric overflow and will yield an unexpected result. This is because the max int value in Java is 2,147,483,647 and yet there are 31,536,000,000 ms in a year. On my machine the above code produces 1,471,228,928 which is obviously not correct.

Instead you need to do this:

private static long ONE_YEAR_AS_MILLISECONDS = 365L*24L*60L*60L*1000L;
Chris Knight