views:

960

answers:

11

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.

A: 

I bet you always get it right from now on.

Skip Head
+1  A: 

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.

Nils Pipenbrinck
The interesting thing is that I've been coding for more the 20 years now, and that's the first time I made that specific mistake. When I was a C/C++ developer I used to pay attention to each little detail on my code. I think that now I'm expecting the compiler to do some "magic" for me...
golimpio
+5  A: 

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).

Jon Skeet
I was expecting a long, I'm used to work with System.currentTimeMillis(). Because that's not a new code, I don't remember what I was thinking when I wrote it (probably I was expecting some magic from the compiler)...
golimpio
...I think that my big mistake, like you said, was doing arithmetic operations with ints, without pay attention to the int overflow. I forgot the basic concepts (in fact I didn't learn it for java, but I had learned for C/C++)
golimpio
+5  A: 

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.

Bill the Lizard
+2  A: 

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.

Martin OConnor
it looks that Java has the same behavior for int overflow that C. I think I was expecting the compiler to convert it (int to long) for me. It's the kind of code that look pretty simple (and it is)...I just have to pay more attention.
golimpio
+2  A: 
Julien Chastang
Thanks, that's a good book.
golimpio
+1  A: 

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.

Avrom
A: 

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

golimpio
+1  A: 

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;
}
Peter Lawrey
This way is better, I prefer to avoid unnecessary casting. thanks
golimpio
+1  A: 

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.

Sean McCauliff
I completely forgot about FindBugs...thanks
golimpio
+1  A: 

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.

hacken