Simple if statement in C always return true

Question

I dont have time to explain it deeply, its very simple code but the function always return 'y'(=true) It is expected to write each number from 1 to squareroot of the generated random number and decide whether it is dividable or not but when i run it, somehow the if statement in the function always return true

#include <stdio.h>
#include <stdlib.h>
int a,b,i;
char c;
char abcd(char c);
int main()
{
srand(time(NULL));
int a=rand()%512;
 b=sqrt(a);
 i=1;
 do{
    if(abcd(c)=='y')printf("number %d is dividable by %d\n",a,i);
    else printf("number %d is not dividable by %d\n",a,i);
    i++;
}while(i<=b);

return 0;
}
char abcd(char c)
{

    if(a%i==0)return'y';
    else return 'n';

}

You don't seem to have time to put spaces, newlines and proper indentation too. Please find this time, if you are asking for ours. — Eugene Sh., May 21 '19 at 20:23
Hint: add the line `fprintf(stderr, "a = %d\ni = %d\n", a, i);` to the start of the function `abcd` and run the program again. — Govind Parmar, May 21 '19 at 20:23
It's a reasonable rule of thumb that single-letter variable names should never be made into global variables — and seldom into file scope (`static` — linkage and duration) variables. All your global variables should be local to `main()`. If you use GCC or Clang to compile, you can use `-Wshadow` to spot problems where local variables (such as the `a` defined in `main()`) shadow or hide file scope or global variables. — Jonathan Leffler, May 22 '19 at 02:59

MFisherKDX · Answer 1 · 2019-05-21T20:40:49.887

2

When you declare int a inside main as

int a=rand()%512;

you are shadowing your global variable a. The a in main is a different variable that has scope only local to the function main. Therefore, when you are using the value a inside char abcd(char c), this value is the global variable a which is default initialized to 0.

Also, why are you passing a char c variable to function abcd. You aren't using it. Please consider renaming your functions to something that more clearly describes their intent.

edited May 21 '19 at 20:40

answered May 21 '19 at 20:34

MFisherKDX

2,840
3
14
25

The behavior is not undefined. Objects at file scope have `static` storage duration, and are initialized to 0 if no explicit initializer is present. – John Bode May 21 '19 at 20:36
you sure you explicitly don't need 'static int a''? – MFisherKDX May 21 '19 at 20:37
objects declared at file scope are `static` by default. – John Bode May 21 '19 at 20:39
1

Be careful, @JohnBode. You are exactly correct, but your comments are susceptible to being misinterpreted as saying that file-scope variables by default have static *linkage*, especially since you seem to be referencing the `static` keyword, which has that effect at file scope. – John Bollinger May 21 '19 at 20:59

score 0 · Answer 2 · edited May 21 '19 at 20:41

The reason yours doesn't work is because the variable a was declared in a separate scope from the abcd function. The a variable you use inside the abcd function is automatically set to 0, which is why it returns true every time (0 % anything is 0). When you call abcd, you would need to pass a inside the parameters for it to use the correct value.

But really you don't need the abcd function, you can save a lot of code and directly check if it's divisible. This code should work:

#include <stdio.h>
#include <stdlib.h>
#include <math.h>
#include <time.h>
int a, b, i;
char c;
int main()
{
    srand(time(NULL));
    int a = rand() % 512;
    b = sqrt(a);
    i = 1;
    do {
        if (a%i == 0)printf("number %d is dividable by %d\n", a, i);
        else printf("number %d is not dividable by %d\n", a, i);
        i++;
    } while (i <= b);

    return 0;
}

I would highly recommend getting rid of the abcd function. Functions should be written either so that the code can be reused, or to increase readability. In this case, it’s really just a waste of lines and increased runtime for the function call. — Daniel C Jacobs, May 22 '19 at 01:13

score 0 · Accepted Answer · edited Jun 20 '20 at 09:12

You have two different variables a:

one declared at file scope
```
int a,b,i;
```
and one declared in main():
```
int a=rand()%512;
```

Within its scope (almost all of main()), the latter shadows the former. Elsewhere, such as in function abcd(), only the former is visible. The former is default initialized to 0 and no other value is ever assigned to it, so no matter what value i takes, inside abcd(), the expression a%i evaluates to 0.

This is a good lesson in avoiding file-scope variables. Functions should operate on data accessed directly or indirectly through their parameters, or obtained from an external source. It is poor form for functions to exchange data through file-scope variables. Moreover, it was a red flag to me that your function abcd() declares a parameter that it never uses. Suggested variation:

char abcd(int dividend, int divisor) {
    return (dividend % divisor) ? 'n' : 'y';
}

Or even better (because better name and more appropriate return type):

_Bool is_divisible(int dividend, int divisor) {
    return !(dividend % divisor);
}

Simple if statement in C always return true

3 Answers3