1

I am writing a code that write all the prime numbers from 2 to 1000 in a file, named primes.txt. For some reason I am not able to figure out the correct way to do this problem. 

import java.io.File;
import java.io.FileNotFoundException;
import java.io.PrintWriter;

public class Problem6 {

    /**
     * @param args
     * @throws FileNotFoundException 
     */
    public static void main(String[] args) throws FileNotFoundException {
        PrintWriter prw = new PrintWriter("primes.txt");
        for (int i = 2; i <= 1000; i++){
            if (checkIfPrime(i) == true){
                System.out.println(i);
                prw.println(i);
            }
        }
    }

    public static boolean checkIfPrime (int num){
        boolean isPrime = true;  
        for (int i = 2; i <= 1000; i++){
            if ( num % i == 0 ){
                isPrime = false;
            }
        }

        return isPrime;
    }
}

I just dont know what to do... Please help Thanks!

Dennis Meng
  • 4,932
  • 14
  • 30
  • 36
CBH
  • 95
  • 1
  • 2
  • 9

5 Answers5

6

What happens when you pass in your first number, 2, to checkIfPrime? It will take remainder of 2 divided by 2, which is 0, falsely claiming that 2 is not prime.

You need to stop testing remainders before you actually get to num. Stop your i for loop before i gets to num. (In fact, you can stop after i has reached the square root of num).

for (int i = 2; i < num; i++){

or even

for (int i = 2; i <= Math.sqrt(num); i++){

If you're feeling adventurous, you may try implementing the Sieve of Eratosthenes, which marks all composite numbers up to an arbitrary limit (in this question, 1000). Then you just print out the rest of the numbers -- the primes.

rgettman
  • 167,281
  • 27
  • 248
  • 326
  • +1 for sqrt variant. It is good to notice that result of dividing `num` by divisor smaller that sqrt will be divisor larger than sqrt, but since we already checked smaller divisors there is no need to checki greater ones. – Pshemo Sep 06 '13 at 22:25
  • The last example isn't entirely trustworthy. It should be OK for `num` in the range 1 to 1000. But suppose we want to deal with large numbers and `num` is a perfect square, `q*q`, that is close to the maximum for a `long`, which is 2^63-1. `Math.sqrt` takes a `double`, which has only 52 mantissa bits, so some precision will be lost. In theory, this means that `Math.sqrt(num)` could be slightly less than `q`, and the comparison as written could fail when `i==q`, which would be incorrect. In practice, I couldn't get this to happen. But one does need to be careful with floats. – ajb Sep 06 '13 at 22:52
  • 2
    P.S. that's why I prefer `for (int i = 2; i * i <= num; i++)` even though it's a bit less readable. Integer arithmetic is exact, so I try to avoid floating-point arithmetic when it isn't needed. – ajb Sep 06 '13 at 22:59
  • @ajb Your loop will only work for `i < 65536`, because the multiplication will overflow. You need to make sure that one of the multiplicands is a long, for example `1L * i * i`. Or declare `i` as `long` for simplicity. – starblue Sep 07 '13 at 09:09
  • 1
    @starblue Sloppy on my part. If `num` is an `int`, though, my code will work fine because `i` can never be > 46341. If `num` is `long`, `i` definitely must also be `long`. But yes, if you use an expression like this in a `for` condition, you have to be really careful about boundary conditions. If this were C++ and `num` were an `unsigned int` then my code could fail. – ajb Sep 07 '13 at 17:36
3

calculation can be made even faster by checking the division with prime numbers only. Any non prime number is divisible by some prime number smaller than itself. 

    static List<Integer> primes = new ArrayList<Integer>();

public static void main(String[] args) {
    for (int i = 2; i < 10000; i++) {
        if(checkPrime(i)){
            primes.add(i);
        }
    }
    System.out.println(primes);
}

private static boolean checkPrime(int n) {
    for (Integer i : primes) {
        if(i*i > n ){
            break;
        }else if(n%i==0 )
            return false;
     }
    return true;
}
Braj Kishore
  • 351
  • 2
  • 11
  • actually breaking from the `for` loop when `i*i <= n` is no longer true should make this code (it works to 10,000) run about 30 times faster (excluding the printing). You can break your `if` in two. – Will Ness Sep 08 '13 at 13:48
  • Thanks @Ness. Updated the code as per your comment. I have not measured the performance. My gut feeling is that as you increase n it will show better performance. – Braj Kishore Sep 09 '13 at 14:47
  • you're welcome. for me, it's more readable this way: `if(i*i > n) { break; } else if(n%i==0) { return false; }`. – Will Ness Sep 11 '13 at 06:13
2

Change your for condition in checkIfPrime(int num) to 

for (int i = 2; i < num; i++) {

BTW if (checkIfPrime(i) == true){ can be written as if (checkIfPrime(i)){

Pshemo
  • 113,402
  • 22
  • 170
  • 242
1

A number num is prime if it's not divisible by any other number that is greater than one and smaller than num. Where is this in your code? :-)

nickie
  • 5,068
  • 2
  • 20
  • 35
0

Here's how you could "hard-code" an incremental sieve of Eratosthenes on a 2-3-5-7 wheel to print the primes up to 1000. In C-like pseudocode,

primes_1000()
{
   // the 2-3-5-7 wheel
   int wh[48] = {10,2,4,2,4,6,2,6,4,2,4,6,6,2,6,4,2,6,4,6,8,4,2,4,
                  2,4,8,6,4,6,2,4,6,2,6,6,4,2,4,6,2,6,4,2,4,2,10,2};
   // core primes' multiples, each with its pointer into the wheel
   int m[7][4] = { {1,11,11,11*11}, {2,13,13,13*13}, {3,17,17,17*17},
                   {4,19,19,19*19}, {5,23,23,23*23}, {6,29,29,29*29},
                   {7,31,31,31*31} };    // 23*23 = 529 
   int i=1, p=11, k=0;
   print(2); print(3); print(5); print(7);
   p = 11;             // first number on the wheel - the first candidate
   do {
      // the smallest duplicate multiple is 121*13, ==> no dups below 1000!
      for( k=0; k < 7; ++k) {
         if ( p == m[k][3] ) {             // p is a multiple of m[k][1] prime:
            m[k][2] += wh[ m[k][0]++ ];    //   next number on the wheel
            m[k][3]  = m[k][1] * m[k][2];  //   next multiple of m[k][1] 
            m[k][0] %= 48;                 //   index into the wheel
            break;
         }
      }
      if (k == 7) {    // multiple of no prime below 32 -
          print(p);    //   - a prime below 1000!   (32^2 = 1024)
      }
      p += wh[i++];    // next number on the candidates wheel
      i %= 48;         // wrap around to simulate circular list
   } while ( p < 1000 );
}

For primes below 500, only 4 sieve variables need to be maintained, for the additional core primes {11,13,17,19} above the wheel's inherent primes 2,3,5,7.

(see also Printing prime numbers from 1 through 100).

m is the dictionary of base primes and their multiples on the wheel (multiplesOf(p) = map( multiplyBy(p), rollWheelFrom(p) ), each with its own index into the wheel. It should really be a priority queue, min-ordered by the multiples' values.

For a true unbounded solution a separate primes supply can be maintained, to extend the dictionary prime by prime when the next prime's square is reached among the candidates.

Community
  • 1
  • 1
Will Ness
  • 62,652
  • 8
  • 86
  • 167