HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Printing twin primes less than a given natural number n

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
numberprimesthanprintingtwinlessnaturalgiven

Problem

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++;
    }

Solution

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 


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.

Code Snippets

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++;
}
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++;
}
#include <stdbool.h>
typedef int bool;
#define true 1
#define false 0
typedef int bool;
enum { false, true };

Context

StackExchange Code Review Q#73362, answer score: 4

Revisions (0)

No revisions yet.