6
\$\begingroup\$

I have two programs doing this task correctly, but at first I'm not sure if the code is correct (I mean absence of not efficient and superfluous pieces of the code). Then, I can't choose which one is better and smarter. Couldn't you do it for me?

First:

int n, p, q;

scanf_s("%d", &n);

p = 1;
q = 2;

char flag = 1;

while (q < n)
{
    flag = 1;
    for (int i = 2; i <= sqrt(q); i++)
    {
        if (q%i == 0)
        {
            flag = 0;
            break;
        }
    }
    if (flag)
    {
        if (q - p == 2)
        printf("%d %d\n", p, q);
        p = q;
    }
    q++;
}

Second:

int n=0, p=3, q=5, f = 0;
    scanf("%d", &n);
    while (q < n)
    {
        f = 1;
        for (int i = 2; i <= p / 2; i++)
            if (p%i==0) { f = 0; break; }

        for (int i = 2; i <= q / 2; i++)
            if (q%i == 0) { f = 0; break; }

        if (f) printf("p = %d, q = %d\n", p, q);
        p++;
        q++;
    }
\$\endgroup\$
4
\$\begingroup\$

The differences between the two pieces of code are piling up.

The ones that I notice right away are the differences in the style or formatting

Let's first format them the same so that we can tell what is actually different.

This is the second piece of code, in which there are still some things I don't like, but we will get to that.

int n, p, q, f;
n = 0;
p = 3;
q = 5;
f = 0;  

scanf("%d", &n);

while (q < n)
{
    f = 1;
    for (int i = 2; i <= p / 2; i++)
    {
        if (p%i==0) 
        { 
            f = 0; break; 
        }
    }
    for (int i = 2; i <= q / 2; i++)
    {
        if (q%i == 0) 
        { 
            f = 0; 
            break; 
        }
    }
    if (f) 
    {
        printf("p = %d, q = %d\n", p, q);
    }
    p++;
    q++;
}

Here you could actually create you f variable inside of the While loop instead of at the top.

Don't one-line if statements so you don't have to use brackets for the For loop. dangerous coding. Just use brackets (curly braces).

What are all these variables? I don't know what they are or what they do. Please use more descriptive variable names.

I don't know what the standard for C is, but in C# you wouldn't declare all your Variables in one line, so this looks bad to me.

Sometimes you put spaces in between operators and variables and sometimes you don't, please use spaces.


The first piece of code looked like it was formatted pretty well. The only thing that I did was to break up the variable declarations and space some things out a little bit.

I would still recommend that you create more memorable variable names though.

int n;
int p;
int q;

scanf_s("%d", &n);

p = 1;
q = 2;

char flag = 1;

while (q < n)
{
    flag = 1;
    for (int i = 2; i <= sqrt(q); i++)
    {
        if (q % i == 0)
        {
            flag = 0;
            break;
        }
    }
    if (flag)
    {
        if (q - p == 2)
        printf("%d %d\n", p, q);
        p = q;
    }
    q++;
}

Not assigning the variable n is probably a really bad idea, definitely not a good habit to fall into for sure.

I have been corrected, it is okay to not assign to n but you don't check the value returned by the scanf and that could result in a null reference exception


One thing that I noticed is that you are using either an integer or a char datatype for a flag, why wouldn't you use a boolean type or even a bit instead?

It looks like you could use an #include if you are in C99.

#include <stdbool.h>

but it does appear that the other options are integers except for one which is an Enum

Option 1

typedef int bool;
#define true 1
#define false 0

Option 2

typedef int bool;
enum { false, true };

Option 3

typedef enum { false, true } bool;

Reference: answer to -> Using boolean values in C

So I guess that it would make sense to use an integer. The char might even make sense since it is an integer type. But I think it would be less confusing if you defined a data type or at the very least label it flag and not f.

\$\endgroup\$
  • 1
    \$\begingroup\$ Not assigning a value to initialize n is fine. However, failing to check the return value of scanf() is bad. \$\endgroup\$ – 200_success Dec 12 '14 at 3:21
  • \$\begingroup\$ Using an int for a true/false variable is common practice in C. C99 bool support is prettier, but it doesn't make a significant difference in practice. \$\endgroup\$ – 200_success Dec 12 '14 at 3:25
3
\$\begingroup\$

The second version seems to be an obfuscated, de-optimized version of the first one. I say "obfuscated" because you took a variable named flag and shortened its name to f. I say "de-optimized" because… well, it does much more work.

But neither one is particularly good. For one, you should only ever need to work with odd numbers. For another, variables named flag are almost always a sign of unclear thinking. A loop structured like this would be much more meaningful.

for (int p = 3, q = 5; q < n; q += 2)
{
    if (is_prime(q))
    {
        if (q - p == 2)
        {
            printf("%d %d\n", p, q);
        }
        p = q;
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.