I get a negative number for the denominator

Question

I wrote a program that adds two fractions and if the denominator is 0 it should throw IllegalArgumentException. When I test it, I get a failure, when I try to add 0/2 + -1/2 I should get -1/2 but instead I get 1/-2, how can I solve this problem?

The language is german, bruch means fraction, neuNenner means new denominator, neuZaehler means new numerator and ggt is the gcd.

I deleted

assertEquals("Zaehler = -1 Nenner = 2",
                rechnen.Rechnen.bruchAddition(0, 2, -1, 2));

but then I got this error java.lang.AssertionError

this is my code:

    public class Rechnen {

    public static String bruchAddition(int z1, int n1, int z2, int n2) {

        int neuZaehler = (z1 * n2) + (z2 * n1);
        int neuNenner = n1 * n2;

        int ggt = ggt(neuZaehler, neuNenner);
        neuZaehler = neuZaehler / ggt;
        neuNenner = neuNenner / ggt;

        if (n1 == 0 || n2 == 0) {
            throw new IllegalArgumentException();
        }
        return ("Zaehler = " + neuZaehler + " Nenner = " + neuNenner);

    }

    static public int ggt(int x, int y) {
        if (y == 0) {
            return x;
        }
        return ggt(y, x % y);
    }
}

this is the JUnit Test Case:

import static org.junit.Assert.*;
import org.junit.Test;
public class RechnenTest {
    @Test
    public void test() {
        assertEquals("Zaehler = 1 Nenner = 1",
                rechnen.Rechnen.bruchAddition(1, 3, 2, 3));
        assertEquals("Zaehler = 1 Nenner = 1",
                rechnen.Rechnen.bruchAddition(5, 8, 3, 8));
        assertEquals("Zaehler = 1 Nenner = 1",
                rechnen.Rechnen.bruchAddition(10, 16, 3, 8));
        assertEquals("Zaehler = 1 Nenner = 3",
                rechnen.Rechnen.bruchAddition(-1, 3, 2, 3));
        assertEquals("Zaehler = -1 Nenner = 2",
                rechnen.Rechnen.bruchAddition(0, 2, -1, 2));
        assertEquals("Zaehler = -2 Nenner = 3",
                rechnen.Rechnen.bruchAddition(-1, 3, 1, -3));
        try {
            rechnen.Rechnen.bruchAddition(1, 1, 1, 0);
            fail();
        } catch (IllegalArgumentException e) {
            assertTrue(true);
        }
        try {
            rechnen.Rechnen.bruchAddition(Integer.MAX_VALUE, 1, 1, 1);
            fail();
        } catch (IllegalArgumentException e) {
            assertTrue(true);
        }
        assertEquals("Zaehler = 1 Nenner = " + Integer.MAX_VALUE,
                rechnen.Rechnen.bruchAddition(0, Integer.MAX_VALUE, 1,
                        Integer.MAX_VALUE));
    }
}

Answer 1

I'm not sure Euclid's algorithm for GCD (ggt) works well with negative numbers. I think you may want the result of ggt to be positive always. It's probably best to make sure ggt is only called with positive integers (or 0 for the numerator):

int ggt = ggt(Math.abs(neuZaehler), Math.abs(neuNenner));

In Java, if x is negative and y is positive, x % y will be negative, which I think is why you're getting the negative results you're getting.

EDIT: To answer the second question (why you're getting an AssertionError): The problem is that you're overflowing. You're adding two fractions whose denominator is Integer.MAX_VALUE, and your algorithm multiplies the two values to get neuNenner. Of course, this will produce a result greater than Integer.MAX_VALUE, and so neuNenner will have the incorrect value, messing everything up. Possible solutions: (1) use long values inside bruchAddition and ggt; (2) use BigInteger, which will let you handle integers of any size; (3) don't test with Integer.MAX_VALUE (try Short.MAX_VALUE instead); (4) change bruchAddition to handle special cases where n1 == n2 (you can then just add the numerators), or n1 is divisible by n2 or vice versa (you can then multiply a numerator by n1/n2 or n2/n1 which would avoid handling numbers larger than n1 or n2).

EDIT 2: To further clarify solution (1): it won't work just to change the declarations from int to long like this:

public static String bruchAddition(int z1, int n1, int z2, int n2) {

    long neuZaehler = (z1 * n2) + (z2 * n1);
    long neuNenner = n1 * n2;

because the multiplications are still done using ints, and will still overflow, before the value is casted to a long. However, I've tested this and it works:

public static String bruchAddition(int z1, int n1, int z2, int n2) {

    long neuZaehler = ((long)z1 * (long)n2) + ((long)z2 * (long)n1);
    long neuNenner = (long)n1 * (long)n2;

to make sure all the computations are done using the larger integer size. Also change the result type and parameters types of the ggt method to long, and change the ggt variable to long, but you don't need to do any other casting.