3
votes

I'm having trouble writing a program that finds the prime numbers between 2-50. My program right now finds "mostly" prime numbers but some non-prime numbers are included. I know there are way more effective methods and strategies for finding prime numbers but I'm trying to first run through every possibility before i move onto more efficient and effective strategies. The way the program is written now if it does find a prime number it prints it multiple times when I only want it to be printed once. Is my program conceptually correct or is my rationale flawed. Why does my program mostly find prime numbers but throw in some non prime numbers? Why does my program print the prime numbers multiple times?

Here is my approach to the problem.

  1. create a for loop to represent potential prime numbers between 2-50. Represent these potential prime numbers by the variable "i". This loop will count down from 50.

  2. Since a number is prime only if there are no other divisible numbers besides itself and 1, i want to create a inner for loop to divide i by every possible number between 2 and i -1 to see if any of these numbers divides evenly into i. Represent these possible divisors by the variable j. If at any point j does divides evenly into i it's not a prime number so I want my inner loop to exit.

  3. If i gets divided by all the numbers of j and there are no numbers that divide evenly into i then that number if prime and want to print that number.

*

import acm.program.*;
public class PrimeNumber extends ConsoleProgram{
public void run(){

  for (int i =50; i >= 2; i--){
    for (int j= 2; j < i-1; j++){

        if (i % j >= 1){
         println(i);
         }else{
         if (i % j == 0) break;
                }
              } /*end of inner loop */  
           } /* end of for loop */

       } /* end of run method */
     } 
7
2 and i -1 this is too much, you can check divisions between 2 and sqrt(i). - cerkiewny
For this algorithm to work, then i%j would have to be >= 1 for ALL the inner loop cases, not just one. As cerkiewny points out, you can reduce the loop iterations by just going to sqrt(i). - lurker
i++ in the inner loop must be a typo? - Radio-
hehe ... comments are faster than answers ... beat me to it. - rolfl
@Radio- No, not a typo. I want the outer loop to count down and the inner loop to count up. So for example to test 50 i want 50 % 2...3...4.. all the way up to 50 % 49. - Jessica M.

7 Answers

2
votes

You made 2 mistakes. The first is explained in the comment by @Thomas and the second is explained in the comment by @rolfl. I corrected them both here:

public class PrimeNumber extends ConsoleProgram {
    public void run() {
        for (int i = 50; i >= 2; i--) {
            boolean isPrime = true;
            for (int j = 2; j < i-1; j++) { //increment "j" not "i"
                if (i % j == 0) {
                    isPrime = false;
                    break;
                }
            }
            if (isPrime) System.out.println(i);
        }
    } 
}

Note: I verified the solution using this code (save in your IDE as PrimeNumber.java):

public class PrimeNumber {
    public static void main (String args[]) {
        for (int i = 50; i >= 2; i--) {
            boolean isPrime = true;
            for (int j = 2; j < i-1; j++) { //increment "j" not "i"
                if (i % j == 0) {
                    isPrime = false;
                    break;
                }
            }
            if (isPrime) System.out.println(i);
        }
    } 
}

Edit: For your understanding, your main problem is the logic here:

for (int j= 2; j < i-1; j++) {
    if (i % j >= 1) {
        println(i);

You are printing i after only checking one possibility.

For example, take i = 7. You have to test i % j for j = 6, 5, 4, 3, and 2 before you can say that i is prime. You can't just test i % j for j = 6, as you have done. To do so, your println statement should come after the for loop, not nested inside of it, so you can test all the possible values of j first.


Edit 2: In response to

Coincidentally enough, the 1st part of the assignment is to write a predicate method that returns true if the number is prime and false if it is not prime using the strategy of brute-force. The 2nd part of the assignment is to find a more efficient way and reworking the 1st part of the assignment to be more efficient. I was trying to solve the problem using just using two for loops to see if i could do it. Can it be done with just two for loops w/o label and continue since my book has not covered that yet?

Try something like this:

public boolean isPrime(int number) {
    for (int i = 2; i < number; i++) {
        if (number % i == 0) {
            return false; //not prime
        }
    }
    return true; //prime
}
1
votes

There is a bug in your second loop (the inner loop).

You should be incrementing j and not i.... i.e. the innner loop should be

for (int j= 2; j < i-1; j++){

and not

for (int j= 2; j < i-1; i++){
1
votes

You correctly observed that if a number i can be evenly divided by a number j, then i % j == 0.

However, you're printing i every time you find a case where i % j >= 0 -- that doesn't mean however thati is prime, because there could be some other j that i can be evenly divided by.

Instead, you should first go through all of your j, and only if none of them gives you == 0, should you consider i prime. You could use a boolean variable isPrime that is initially true, but that the inner for-loop sets to false as soon as it finds a j by which i can be evenly divided.

0
votes

This if statement:

if (i % j >= 1){
         println(i);

Will print every number that is not divided by j number, and the numbers you want to print are not those that will get a value of division different from 0 from one substract but for ALL substracts.

Also the faster solution is taking the Boolean array of potentially prime numbers and looping like this.

for ( i = 2; i < sqrt(maxvalue); i ++){
            for(j = 1; j < sqrt(maxval); j++){
                  potentialy_prime[i*j]=false
            }
       }
for (potentialy_prime){
    if(potentialy_prime[i]==true)
            print(i "is prime")
}
0
votes

There are 3 problems with your code :

  • In the inner loop, you are increasing i instead of increasing j.

  • You print the number as soon as i % j >= 1, but maybe the next j will divide i, and thus i isn't prime.

  • You should stop the inner loop at sqrt(i) to avoid useless tests.

Here is a corrected version of the code :

OUTER: for (int i = 50; i >= 2; i--) {
    for (int j = 2; j <= (int) Math.sqrt(i); j++) {
        if (i % j == 0)
            continue OUTER;
        } /* end of inner loop */
    println(i);
} /* end of for loop */
0
votes

This code should print out the prime numbers. Initial assumption is that every integer >= 2 is prime(true in boolean array). while traversing through the array, if the number is found to be prime, cross out multiples of that number by setting them false in boolean array ( as they are not prime). At the end the numbers having true in boolean array will be prime numbers only.

Time complexity : O(n log(log n))

public class PrimesImprovement
{
    public static void main(String[] args)
    {  
        System.out.println("Printing primes from 1 to 100");
        int i;
        int j;
        //boolean prime;
        boolean[] prime_arr=new boolean[100];
        Arrays.fill(prime_arr,true); // Assumption
        prime_arr[0]=prime_arr[1]=false;  // As 0 and 1 are not prime numbers
        for ( i=2;i<prime_arr.length;i++) 
        {

            if(prime_arr[i])
            { 
                for ( j=2;i*j<prime_arr.length;j++) 
                {
                    prime_arr[i*j]=false;
                }
            }
        }

        for( i=0; i<prime_arr.length; i++)
        {
            if(prime_arr[i])
            {
                System.out.print(i+" ");
            }
        }

        System.out.println();
    }
}