Proof Reading Please?

[DrExd]

First time attempting this problem. It takes a value and calculated prime numbers up to that value. The program works, but I am worried I might have not used the best method possible, or that there may be other errors in the way I carried out the process. Any advice, pointers and/or helpful comments are highly appreciated.

#include <iostream>
#include <cmath>
using namespace std;

int main()
{
   int x, y, counter = 1, n;

   cout << "Enter the nth prime term you would like to find: " ;
   cin >> n;

   cout << endl;

   for(x = 2; n >= counter; x++)
   {
       bool prime = true;

       for(y = 2; y <= sqrt(x); y++)
       {
           if(x % y == 0 && x != y)
           prime = false;
       }

       if(prime)
       {
           if(x < 10)
           cout << " " ;

           cout << "Prime = " << x << " \tCounter = " << counter << endl;
           counter++;
       }
   }
   return 0;
}

[Jikax]

i think it is alright...

only 1 thing i can think of:

for(x = 2; n >= counter; x++)
you are updating "counter" in the loop not n... my first thought was you were updating "n".
maybe turn it around:
for(x = 2; counter <= n; x++)

It in't wrong(maybe it is just me;) ), but it is easier to read...

[TheIdeasMan]

Hi DrExd, just wondering if this bit could cause a problem

y <= sqrt(x)

x and y are int's, so doing sqrt on them changes them to doubles, so now you have a float comparison, which won't always work, given how floats are represented.

Might be best to stick to integer types in the for loop, so calc sqrt(x) before the loop and cast it to an int, then use it in the for loop.

Also, I never use single letter as a variable name, it is much clear to use a meaningful word. My other mantra is "Declare, initialise, and comment each variable on it's own line." .

Hope this helps

[Gaminic]

On the used method:
a) Even numbers, aside from 2, will never be primes. You should start from x = 3 and increment it by 2 each time. Start your counter at 1 so you don't forget "2" is a prime.

b) Your inner loop does a lot of useless work. If a number is divisible by 4, it is also divisible by 2. If it is divisible by 15, it is also divisible by 3 and 5. Instead of checking for every y [2, sqrt(x)], you should only check divisibility by the primes in [2, sqrt(x)].

The easiest way is to keep a list. You know how many primes there will be ('n'), so you can allocate memory from the start. You know how many primes there are at any given moment ('counter'), so you have a loop condition.


Jikax makes a good point in terms of readability. Generally, we like to keep the LHS variable and the RHS constant. Alternatively, as 'counter' and 'x' are completely unrelated variables, it might be better to use a while loop. The loop quits when counter >= n, regardless of the value of 'x'. Your loop should make that clear!

[kbw]

1. y <= sqrt(x) will be subject to rounding errors, it may be better to check (y*y) <= x instead.

2. You only need to keep going around that loop why prime is true.

3. there's no point checking even numbers other than 2,

4. You can use stream manipulators to format the output. So instead of:

           if(x < 10)
           cout << " " ;

           cout << "Prime = " << x << " \tCounter = " << counter << endl;

You can write:

           cout << "Prime = " << setw(2) << setfill(' ') << x << " \tCounter = " << counter << endl;

You will need:

#include <iomanip> 

If you wanted to be pedantic, the width isn't 2, it's really is a function of n, 1 + log n, which is:

1 + log10(n);

where for log10 you'll need:

#include <math.h> 

[DrExd]

Thank you all for your comments and tips. They are very helpful to a beginner such as myself. I will heed all of your advice as I try to perfect this simple project, Thanks again! :)