Merge pull request #25 from joe-bookwood/multiplication-error-fix

Multiplication error fix and additional tests for the add method of FastMoney

Author: keilw

src/main/java/org/javamoney/moneta/FastMoney.java

@@ -450,24 +450,38 @@ public FastMoney multiply(Number multiplicand) {
     }
 
     private long multiplyExact(long num1, long num2) {
-        if(num1==0){
-            return num2;
-        }
-        if(num2==0){
-            return num1;
+        if(num1==0 || num2==0){
+            return 0;
         }
-        boolean pos = num1>0 && num2 >0;
-        boolean neg = num1<0 && num2 <0;
+        
+         // Hacker's Delight, 
+        int leadingZeros =
+                Long.numberOfLeadingZeros(num1) +
+                Long.numberOfLeadingZeros(~num1) +
+                Long.numberOfLeadingZeros(num2) +
+                Long.numberOfLeadingZeros(~num2);
+        
+        if (leadingZeros > Long.SIZE + 1 ) {
+            // in this case, an overflow is impossible
+            return num1 * num2;
+        }
+
+        if (leadingZeros < Long.SIZE){
+        	if(Long.signum(num1)*Long.signum(num2) > 0){
+        		throw new ArithmeticException("Long evaluation positive overflow.");
+           	} else {
+        		throw new ArithmeticException("Long evaluation negative overflow.");
+        	}
+        }
+        
         long exact = num1 * num2;
-        if(pos && exact <=0){
-            throw new ArithmeticException("Long evaluation positive overflow.");
-        }
-        if(neg && exact <=0){
-            throw new ArithmeticException("Long evaluation negative overflow.");
-        }
-        if(!neg && !pos && exact >=0){
-            throw new ArithmeticException("Long negative overflow.");
+
+        // very expensive - this check is only executed in edge cases
+        // zero check is not needed, see above
+        if( exact / num1 != num2 ) {
+        	throw new ArithmeticException("overflow");
         }
+        
         return exact;
     }
 

src/test/java/org/javamoney/moneta/FastMoneyTest.java

@@ -15,6 +15,24 @@
  */
 package org.javamoney.moneta;
 
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertFalse;
+import static org.testng.Assert.assertNotNull;
+import static org.testng.Assert.assertNotSame;
+import static org.testng.Assert.assertTrue;
+import static org.testng.Assert.fail;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.ObjectInputStream;
+import java.io.ObjectOutputStream;
+import java.lang.invoke.MethodHandles;
+import java.math.BigDecimal;
+import java.math.BigInteger;
+import java.util.logging.Level;
+import java.util.logging.Logger;
+
 import javax.money.CurrencyUnit;
 import javax.money.Monetary;
 import javax.money.MonetaryAmount;
@@ -24,15 +42,6 @@
 import org.junit.Assert;
 import org.testng.annotations.Test;
 
-import java.io.*;
-import java.lang.invoke.MethodHandles;
-import java.math.BigDecimal;
-import java.math.BigInteger;
-import java.util.logging.Level;
-import java.util.logging.Logger;
-
-import static org.testng.Assert.*;
-
 /**
  * @author Anatole
  */
@@ -281,7 +290,22 @@ public void testAdd(){
         assertNotNull(moneyResult);
         assertEquals(11d, moneyResult.getNumber().doubleValue(), 0d);
 
-        FastMoney money3 = FastMoney.of(90000000000000L, "CHF");
+        // This example create a sum that is to big for fastmoney, it should overflow
+        // 87978089321359 + 4866358678300 = 92844447999659 > 92233720368547.75807
+        money1 = FastMoney.of(87978089321359L, EURO);
+        money2 = FastMoney.of(4866358678300L, EURO);
+
+        try {
+            moneyResult = money1.add(money2); 
+            fail("overflow should raise ArithmeticException");
+        } catch (ArithmeticException e) {
+            // should happen
+        }
+        
+        // check greates FM 92233720368547.75807 value
+        long fastMoneyMax = 92233720368547L;
+		FastMoney money3 = FastMoney.of(fastMoneyMax, "CHF");
+        
         try {
             // the maximum value for FastMoney is 92233720368547.75807 so this should overflow
             money3.add(money3);
@@ -409,14 +433,29 @@ public void testMultiplyLong(){
         assertEquals(FastMoney.of(200, "CHF"), m.multiply(2));
         assertEquals(FastMoney.of(new BigDecimal("50.0"), "CHF"), m.multiply(0.5));
 
+        // Zero test
+        m = FastMoney.of(100, "CHF");
+        assertEquals( m.multiply(0), FastMoney.of(0, "CHF"));
+        m = FastMoney.of(0, "CHF");
+        assertEquals( m.multiply(10), FastMoney.of(0, "CHF"));
+        
         try {
             // the maximum value for FastMoney is 92233720368547.75807 so this should overflow
             FastMoney.of(90000000000000L, "CHF").multiply(90000000000000L);
             fail("overflow should raise ArithmeticException");
         } catch (ArithmeticException e) {
             // should happen
         }
-    }
+        try {
+            // the maximum value for FastMoney is 92233720368547.75807 
+            // these values are lower, but the overflow detection does not work
+            // correct.
+            FastMoney.of(-53484567177043L, "CHF").multiply(2178802625L);
+            fail("overflow should raise ArithmeticException");
+        } catch (ArithmeticException e) {
+            // should happen
+        }
+   }
 
     /**
      * Test method for {@link FastMoney#multiply(double)}.
@@ -562,6 +601,9 @@ public void testNegate(){
         } catch (ArithmeticException e) {
             // should happen
         }
+        
+        m = FastMoney.of(0, "CHF");
+        assertEquals(m.negate(), FastMoney.of(0, "CHF"));
     }
 
     /**