what is the mistake in printing prime numbers between two numbers?

Question

i tried writing code in c++ for printing prime no.s between 2 numbers . But the output is not satisfactory as it prints the no. 2 - 3 times and also shows non prime numbers.

#include <iostream>

using namespace std;

int n;

int main()
{
    int a, b;
    cout << "enter the first no." << endl;
    cin >> a;

    cout << "Enter the second no." << endl;
    cin >> b;

    for (int i = a; i <= b; i++)
    {
        for (int n = 2; n < i; n++)
        {
            if (i % n == 0)
            {
                break; 
            }
    
            cout << n << endl;  
        }   
    }

    return 0;
}

i was expecting the prime no. but it is not coming.

score 1 · Answer 1 · answered Sep 03 '23 at 06:28

You have declared n outside the scope, but within the second for loop you have redeclared it again (int n = ...). This n and the n outside the main function are different. Also, while checking whether your number is prime, you are printing n which is why you are getting multiple numbers (those numbers correspond to the value of n in a particular iteration). Use a flag (a boolean perhaps) to check if a number is prime or not, and then check outside the second loop and print accordingly. The corrected code is given below:

#include <iostream>
using namespace std;
int n;
int main()
{
    int a, b;
    cout<<"enter the first no."<<endl;
    cin>>a;

    cout<<"Enter the second no."<<endl;
    cin>>b;

    for (int i = a; i <=b; i++)
    {
        bool isprime = true;
        for (n = 2; n < i; n++) /* notice no int n = 2 */
        {
            if (i%n==0)
            {
                isprime = false;
                break;
            }
        }
        if (isprime)
            cout << n << endl;
    }
    return 0;
}

tbxfreeware · Answer 2 · 2023-09-03T09:20:47.097

In general, global variables are problematic

As was noted in the comments, variable n is declared twice in your program, first as a global variable, and then later, as a local variable. After its declaration, global n is never referenced again. All computation and output comes from local n.

You can prove this by simply deleting the global declaration. In general, global variables are problematic in large projects, anyway, so getting rid of global n is probably a good idea.

While I'm at it, I am going to get rid of using namespace std. Professionals rarely use it. Instead, they type out std:: every time they reference a name from the Standard Library.

With those changes, your program becomes:

#include <iostream>

int main()
{
    int a, b;
    std::cout << "enter the first no." << std::endl;
    std::cin >> a;

    std::cout << "Enter the second no." << std::endl;
    std::cin >> b;

    for (int i = a; i <= b; i++)
    {
        for (int n = 2; n < i; n++)
        {
            if (i % n == 0)
            {
                break;
            }

            std::cout << n << std::endl;
        }
    }
    return 0;
}

I ran it, attempting to find prime numbers between 1 and 10.

Here is the output:

enter the first no.
1
Enter the second no.
10
2
2
3
4
2
3
4
5
6
2

To help make sense of the output, I added the following output statement at the top of the outer loop.

    for (int i = a; i <= b; i++)
    {
        std::cout << "Checking i: Is " << i << " prime?\n";
        for (int n = 2; n < i; n++)
        {
            //...

This give me some more clues about what is going wrong:

enter the first no.
1
Enter the second no.
10
Checking i: Is 1 prime?
Checking i: Is 2 prime?
Checking i: Is 3 prime?
2
Checking i: Is 4 prime?
Checking i: Is 5 prime?
2
3
4
Checking i: Is 6 prime?
Checking i: Is 7 prime?
2
3
4
5
6
Checking i: Is 8 prime?
Checking i: Is 9 prime?
2
Checking i: Is 10 prime?

This is too much output. When checking 7, for instance, the only output should be 7 itself. Instead, the output is a list of all the values checked in the inner n loop. Ditto for 3 and 5.

Evidently, the std::cout statement in the n loop should not be there. Why am I outputting n at all? Variable i is the one I care about. I want to know whether i is a prime number, not n.

This version outputs i, instead of n. It also moves the std::cout statement out of the nloop, placing it just below that loop.

    for (int i = a; i <= b; i++)
    {
        std::cout << "Checking i: Is " << i << " prime?\n";
        for (int n = 2;; n < i; n++)
        {
            if (i % n == 0)
            {
                break;
            }
        }
        std::cout << i << std::endl;
    }

Now, I get this output:

enter the first no.
1
Enter the second no.
10
Checking i: Is 1 prime?
1
Checking i: Is 2 prime?
2
Checking i: Is 3 prime?
3
Checking i: Is 4 prime?
4
Checking i: Is 5 prime?
5
Checking i: Is 6 prime?
6
Checking i: Is 7 prime?
7
Checking i: Is 8 prime?
8
Checking i: Is 9 prime?
9
Checking i: Is 10 prime?
10

That does not seem like much progress! Now, everything is prime. The good news is that the right number is being output. When testing 7, for instance, 7 is what gets printed.

Single responsiblity

At this point, I want to isolate the test for prime numbers in a separate function. As things stand, function main is doing three things:

Input and output
Run a "driver" loop which selects which numbers to check
Test whether a given number is prime

When things go wrong, it is hard to debug, because the three activities are all mixed together.

A good general principle is to limit each function to a single responsibility. In this case, I want to create a function is_prime that returns a bool. To start things out, I will just return true for every number:

bool is_prime(int const n)
{
    return true;
}

This greatly simplifies function main.

    for (int i = a; i <= b; i++)
    {
        std::cout << "Checking i: Is " << i << " prime?\n";
        for (int n = 2; n < i; n++)
        {
            if (is_prime(n))
            {
                std::cout << i << std::endl;
            }
        }
    }

Because the stub function is_prime returns true for every number, the output is exactly the same as it was before. Everything is prime.

Fortunately, that is easy to fix. The trick is to return false the first time n % j is zero. If you make it all the way to the end of the loop without returning false, then n must be prime. In that case, return true.

I also put a special case right at the top: Anything less than 2 is not prime, so return false.

bool is_prime(int const n)
{
    if (n < 2)
    {
        return false;
    }
    for (int j = 2; j < n; ++j)
    {
        if (n % j == 0)
        {
            return false;
        }
    }
    return true;
}

The complete program

For clarity, here is the finished program, all in one place. I have removed the extra output statment I inserted earlier, at the top of the i loop. Instead, I print a simple header.

#include <iostream>

bool is_prime(int const n)
{
    if (n < 2)
    {
        return false;
    }
    for (int j = 2; j < n; ++j)
    {
        if (n % j == 0)
        {
            return false;
        }
    }
    return true;
}
int main()
{
    int a, b;
    std::cout << "enter the first no." << std::endl;
    std::cin >> a;

    std::cout << "Enter the second no." << std::endl;
    std::cin >> b;

    std::cout << "\nPrime numbers between " 
        << a << " and " << b << ":\n";
    for (int i = a; i <= b; i++)
    {
        if (is_prime(i))
        {
            std::cout << i << std::endl;
        }
    }
    return 0;
}

And here is the output it generates:

enter the first no.
1
Enter the second no.
10

Prime numbers between 1 and 10:
2
3
5
7

what is the mistake in printing prime numbers between two numbers?

2 Answers2

In general, global variables are problematic

Single responsiblity

The complete program