Implementation of sinus function | Calculation Time

Question

Can anybody tell me why this code doesn't work? I'm trying to implement a sinus-function with a given limitation of precision (by the error-variable).

Certain details about the calculation are given here: https://en.wikipedia.org/wiki/Taylor_series

berechnePot calculates the potence berechneFak calculates the faculty berechneVZW calculates the pre sign (plus or minus)

I don't get the point, why the function is calculating that slow.

public class Sinus {

    public static double sinus(double x) {
        double error = 0.001;

        double summand = 0;
        double sum = 0;

        int k = 0;
        do {
            double pot = 0;
            double fak = 0;
            pot = berechnePot(x, k);
            fak = berechneFak(k);
            summand = pot / fak;
            berechneVZW(summand, k);
            sum += summand;

            k++;
        } while (abs(summand) > error);
        return sum;
    }

    public static double abs(double value) {
        if (value < 0) return -value;
        else return value;
    }

    public static double berechneVZW(double value, int k) {
        if (k % 2 == 0) {
            return value;
        } else {
            return value *= (-1);
        }
    }

    public static double berechnePot(double x, double k) {
        double pot = 0;
        pot += x;
        for (int i = 0; i <= k; i++) {
            pot *= (x * x);
        }
        return pot;
    }

    public static double berechneFak(int k) {
        double fak = 1;
        if (k == 0) {
            return 1;
        } else {
            for (int i = 0; i <= k; k++) {
                fak *= (2 * i + 1);
            }
        }
        return fak;

    }
}

Finally i got to the right solution..

I hope that the new structure helps you to better understand my implementation better.

Thanks for all your help!

public class Sinus {

public static double sinus(double x) {
    double error = 0.00001;

    double summand = 0;
    double result = 0;
    double fak = 0;

    int k = 0;
    do {
        double pot = 0;
        pot = calcNumeratorPotency(x, k);
        fak = calcDenumeratorFaculty(k);
        summand = pot / fak;
        summand = definePreSign(summand, k);
        result += summand;
        k++;
    } while (absoluteValue(summand) > error);
    return result;
}

public static double absoluteValue(double value) {
    if (value < 0)
        return -value;
    else
        return value;
}

public static double definePreSign(double value, int k) {
    if (k % 2 == 0) {
        return value;
    } else {
        return value * (-1);
    }
}

public static double calcNumeratorPotency(double x, double k) {
    double pot = x;
    for (int i = 0; i < 2 * k; i++) {
        pot *= x;
    }
    return pot;
}

public static double calcDenumeratorFaculty(int k) {
    double fak = 1;
    if (k == 0) {
        return 1;
    } else {
        for (int i = 1; i <= (2 * k + 1); i++) {
            fak *= i;
        }
    }
    return fak;

}

Answer 1

You seem to have worked with another language before. Java works a bit different than you seem to expect.

for (int i = 0; i <= k; k++) {
    fak *= (2 * i + 1);
}

This particular loop is definetly not working as expected. You increment k, but iis supposed to grow? Might be you want to write:

for (int i = 0; i <= k; i++) {
    fak *= (2 * i + 1);
}

Because the loop counts k, instead of i, it continues, until k overruns the integer-range and becomes Integer.MIN_VALUE. At that point, your loop finally terminates. :)

On a completely different note, and meant as constructive critique: You might want to take a look at the default Java-Style-Guide (https://github.com/twitter/commons/blob/master/src/java/com/twitter/common/styleguide.md)

Small excerpt:

// Bad.
//   - This offers poor visual separation of operations.
int foo=a+b+1;

// Good.
int foo = a + b + 1;

Spaces between identifiers and operators are very, very helpful, operating with numbers, but also with a lot of different stuff.

Answer 2

In your berechneFak() function you have a loop inside the else clause. You do increment k but not i so i <= k is always true. Try looking at it in a debugger. This is the loop that is slowing it down.

So every time k will count up to the max integer value of 2,147,483,647 in single increments and then overflow to a negative value at which point the loop will end.

Just to clarify: I did not look at if the math is correct but just at why the program is slow.

Answer 3

It is not quite clear to me what you are calculating in berechnePot and berechnePot. They seem to be the numerators and denominators, judging from the context.

Running your code with a debugger, I can see that summand is calculated very wrongly, and decreases very slowly. This is the reason why it takes so long.

The Math class provides a pow method, so you don't really need to use a for loop to calculate powers. I think you might be overcomplicating this a bit. I would write a getSummand method and a factorial method:

private static double getSummand(double x, int k) {
    int power = k * 2 + 1;
    double numerator = Math.pow(x, power);
    double denominator = factorial(power);
    int sign = k % 2 == 1 ? -1 : 1;
    return numerator / denominator * sign;
}

private static double factorial(int x) {
    double result = 1;
    for (int i = 1; i <= x; i++) {
        result *= i;
    }
    return result;
}

And use them like this:

public static double sinus(double x) {
    double error = 0.001;

    double summand = 0;
    double sum = 0;

    int k = 0;
    do {
        summand = getSummand(x, k);
        sum += summand;

        k++;
    } while (Math.abs(summand) > error);
    return sum;
}

If this is an assignment and you are not allowed to use anything from the Math class, you could write your own pow method like this:

private static double pow(double x, int p) {
    double result = 1;
    for (int i = 0 ; i < p ; i++) {
        result *= x;
    }
    return result;
}