c++ if statements always return true

Question

I have a bunch of maths manipulations that have thresholds, yet no matter what I change, the if statements always return true. No compile errors, can't get the debugger to work. This is a function, the X Y and Z arrays are all correct (I printed them to check earlier), the maths is right at least for the blade distance check, yet that always returns true. I ran the same code (rewritten obviously) through matlab and that returns true or false depending on my data, so clearly its something with the way I've written this that's wrong. Also is there any way to slimline this?

bool Device::_SafeD(char _Type, float _Data[20][3]) {
    bool S;
    double x[20], y[20], z[20];
    for (int i=0; i<20; i++) {
        x[i] = _Data[i][0];
        y[i] = _Data[i][1];
        z[i] = _Data[i][2];
    }

    // Check angles for needle
    if (_Type == 'n') {
        for (int i=0; i<20; i++) {
            float dot, moda, modb, c, angle;
            dot = ((x[i]*x[i+1]) + (y[i]*y[i+1]) + (z[i]*z[i+1]));
            moda = sqrt(pow(x[i],2)+pow(y[i],2)+pow(z[i],2));
            modb = sqrt(pow(x[i+1],2)+(y[i+1],2)+(z[i+1],2));
            c = dot/(moda*modb);
            angle = acos(c);

            if (angle > 45){
                S = 0;
            } else {
                S = 1;
            }
        }
    }

    // Check distance for blade
    if (_Type == 'b'){
        for (int i=0; i<19; i++) {
            float distance = (x[i+1]-x[i]) + (y[i+1]-y[i]) + (z[i+1]-z[i]);
            cout << "distance " << distance << endl;

            if (distance > 5.0) {
                S  = 0;
            } else {
                S =  1;
            }
        }
    }
    if (S == 0) {
        return 0;
    }
    if(S == 1) {
        return 1;
    }
}

Cheers

It is probably not the issue but any name starting with an underscore and a capital letter like `_Type` is reserved for the implementation and should not be used. — NathanOliver, Mar 30 '16 at 20:02
`acos()` will generally return values in the range [-pi, pi]. I don't think you'll ever see it exceed 45 as your code suggests. — Logicrat, Mar 30 '16 at 20:02
@NathanOliver I've been taught to declare class attributes like that. Granted function shouldnt be but I cba to change it until it works... — Daria D, Mar 30 '16 at 20:04
You're not clear which of the if-statements you're talking about. However, I notice you have "angle > 45", but std::acos() returns a result in radians, not degrees, so that is a suspicious comparison. — Steven, Mar 30 '16 at 20:04
'*can't get the debugger to work*' That's where you should focus your efforts. It's an essential tool. — Biffen, Mar 30 '16 at 20:06
@Steven just did a radian conversion, still always returns 1. — Daria D, Mar 30 '16 at 20:09
@DariaD still unrelated, but you can tell whoever taught you that [**they're wrong**](https://stackoverflow.com/questions/228783/what-are-the-rules-about-using-an-underscore-in-a-c-identifier). — WhozCraig, Mar 30 '16 at 20:11
It's probably unrelated, but your fist loop should end at 19, due to the i+1 dereference. The code also, change the value of S at every iteration, shouldn't it break from the loop? — Bob__, Mar 30 '16 at 20:17
Don't use `pow(x,2)`. Multiply the two values. There is an overhead when calling the `pow` function as well as using the internal algorithm. A single multiplication is far more efficient. — Thomas Matthews, Mar 30 '16 at 20:21
Your for statement loops outside of your if statement for S. As a result, the S variable that you check for after the end of for loop stores the result of S at the end of the array. Hope that helps. — jimmu, Mar 30 '16 at 20:23
Printing key variable to `stdout` is always a debugging option. Add a line `std::cout << "angle: " << angle << std::endl;` just after computing `angle`. — R Sahu, Mar 30 '16 at 20:24
And if S can only be 0 or 1, what is the use of the final two if-statements? Why not just write `return S;` instead? — FredK, Mar 30 '16 at 20:25
OT: An array of `struct Point {int x, y, z;};` would be more efficient than 3 arrays. The data cache of a processor likes to have its data very close. For example the value `y[0]` may be far enough away from `x[0]` to require another data fetch. Whereas `Point[0].x` and `Point[0].y` are guaranteed to be close to each other to not require another data cache fetch. — Thomas Matthews, Mar 30 '16 at 20:25
You should be using `true` or `false` with a `bool` type, not 0 or 1, which are integers. — Thomas Matthews, Mar 30 '16 at 20:27

R Sahu · Answer 1 · 2016-03-30T20:45:30.750

0

The most likely error is that you are comparing angle to an angle in degree while the return value of acos is in radians.

        if (angle > 45){

Convert the angle to degrees before comparing to 45 and you should be OK.

        if (radians_to_degrees(angle) > 45){

where

double radians_to_degrees(double in)
{
    return (in*180/M_PI);
}

Another option is to compute the equivalent of 45 degrees in radians and compare it with angle.

double const radian_45 = M_PI*45.0/180;

and use

        if (angle > radian_45){

The other error is spelled out clearly in a comment by @Bob__:

OP uses two loops to check the angle and the distance, inside those loops a flag (S) is set to 0 or 1, but it's done for every index in the array, so it's overwritten. The return value of the entire function (in the provided code) depends only by the last two elements in the array.

edited Mar 30 '16 at 20:45

answered Mar 30 '16 at 20:28

R Sahu

204,454
14
159
270

3

Maybe a better idea is to use the radian constant for 45 degrees instead of calling a function for conversion. The constant could be declared as `45.0*180/M_PI`. – Thomas Matthews Mar 30 '16 at 20:30
and it should `return 0;` from the loops (not set `S = 0;`) if `angle > 45` or `distance > 5` – Bob__ Mar 30 '16 at 20:30
@Bob__, that didn't make sense. Care to elaborate? – R Sahu Mar 30 '16 at 20:34
OP uses two loops to check the angle and the distance, inside those loops a flag (S) is set to 0 or 1, but it's done for every index in the array, so it's overwritten. The return value of the entire function (in the provided code) depends only by the last two elements in the array. – Bob__ Mar 30 '16 at 20:39
@Bob__, thanks for the clarification. It makes sense now. – R Sahu Mar 30 '16 at 20:42
@DariaD, The SO way of expressing that an answer has helped you is to **accept** it. This tells future readers exactly what was the problem at a glance. And it comes with a nice rep boost to both you and the person that helped you. – StoryTeller - Unslander Monica Mar 30 '16 at 22:36

c++ if statements always return true

1 Answers1