Question

I wrote a method to convert a given number from days to milliseconds:

private long expireTimeInMilliseconds;
...
public void setExpireTimeInDays(int expireTimeInDays)
{
   expireTimeInMilliseconds = expireTimeInDays * 24 * 60 * 60 * 1000;
}

I had a hard time to figure out what I did wrong. Now my question: Is that error so obvious ?

The corrected method:

private long expireTimeInMilliseconds;
...
public void setExpireTimeInDays(int expireTimeInDays)
{
   expireTimeInMilliseconds = ((long) expireTimeInDays) * 24 * 60 * 60 * 1000;
}

If I don't convert the integer to long before calculating, I get a complete wrong result.

Was it helpful?

Solution

Is it obvious? I guess it depends on how long you've been using Java and how many times you've had to deal with milliseconds. Of course, it should be okay for up to about 24 days...

I think the biggest hint should be that System.currentTimeMillis() returns a long. That's a good indication that a number of milliseconds can get big. The type of the variable you're setting should be a good hint too.

Of course, you've also got to know that if you do arithmetic operations with ints, the result will be int with wrap-around on overflow. Whether that's sufficiently obvious or not could be debated, but it would be a pretty pointless discussion. In C# if you turned overflow checking on, you'd have found the bug pretty quickly - but then not many developers do that (indeed, I don't although I probably should).

OTHER TIPS

Yes, it's pretty obvious if you've done it before. Any time you see a string of numbers multiplied out you should automatically start thinking about integer overflow errors. In this case you're set to overflow if expireTimeInDays is more than 24. Technically you should be thinking about overflow errors any time you're working with integers, but multiplying a group of them like this should be a very big red flag.

Your operand variable and the literal numbers are of type int. The int data type has a maximum value of 2^31 -1. Therefore with such large numbers, the data type of int overflows leading to a seeming incorrect answer.

In your first example, the int is only promoted to a long on assignment to the variable which occurs after the calculation. The result of the calculation is an int.

The second example, casts the first operand to a long, causing the promotion of the calculation to a long. In this case, the result of the calculation is a long, due to promotion. The long data type is more than large enough for your calculation.

You may be interested to know that this is covered in "Java Puzzlers" by Joshua Bloch and Neal Gafter.

alt text
(source: javapuzzlers.com)

You will find many other Java pitfalls, traps and corner cases in that book.

I agree with the starblue who left a comment. Append an L to the number.

No, it's not obvious.

But trust me, after some more years of practice and fixing bugs like this you become very sensible about integer overflows and just do the right thing without even thinking about it.

It's something that happend to everyone. Definately no sign of bad code practice, ignorance or so.

Just to add to the other answers, I have found it helpful in the past to define constants (public static final long) such as MILLISECS_DAY or MILLISECS_HOUR. Much more readable and useful.

Another way to write this is

public void setExpireTimeInDays(int expireTimeInDays)
{
   expireTimeInMilliseconds = (long) expireTimeInDays * 24 * 60 * 60 * 1000;
}

or

public void setExpireTimeInDays(int expireTimeInDays)
{
   expireTimeInMilliseconds = expireTimeInDays * 24L * 60 * 60 * 1000;
}

If you use FindBugs on your code it will detect this exact problem. "ICAST: Result of integer multiplication cast to long." FindBugs' example is exactly what you are doing; calculating days in milliseconds.

This problem was not obvious to me the first time I ran into it.

There are some static analysis tool (findbugs) that will find these type of errors.

Numerical math on computers can be hard. Order of operation matters can affect precision and accuracy in ways that you don't expect. Date math can also be surprisingly tricky. Often it is better to use the Date/Calendar routines rather than trying to do the math yourself but those routines are not the best designed ones in the java class library.

I'm not trying to justify my mistake, but it would be great if the java compiler was smart enough to promote the int to a long before the calculation (once the calculation is being assigned to a variable of type long)

By the way, I used to work with C/C++ and if it was a C program, I'd had the same problem, but some years ago I'd been more careful with this kind of operation.

I'll pay more attention next time (or switch to python)... :D

Licensed under: CC-BY-SA with attribution
Not affiliated with StackOverflow
scroll top