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

Sieve of Eratosthenes and twin prime finder

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

Problem

I made this program to find primes and twin pairs. I'd like feedback on anything from formatting to content to technique. In short: how would this be different if it was written by an experienced professional?

The program runs five methods:

  • getMax asks for an integer input which is the maximum number below which to find primes and twin prime pairs. It also has an error message to catch unacceptable input and ask again.



  • sieve checks every number between 0 and max and returns a boolean array which says whether each index is prime or not.



  • primesCount counts many primes there are, and prints it.



  • primesList creates an integer array of all primes, and prints it.



  • pairsCount counts how many twin prime pairs there are, and prints it.



  • pairsList creates a string array of all twin prime pairs, and prints it.



Code:

```
public static void main(String[] args) {

Scanner in = new Scanner(System.in);
int max = getMax(in);

boolean[] primesSieve = sieve(max);
int primesCount = primesCount(primesSieve);
int[] primesList = primesList(primesSieve, primesCount);
int pairsCount = pairsCount(primesList);
pairsList(primesList, pairsCount);

}

// User input for maximum number to sieve
public static int getMax(Scanner in) {
System.out.print("Maximum number to sieve: ");
while (!in.hasNextInt()) {
String input = in.next();
System.out.println("Error: \"" + input + "\" is not a number. Try again.");
System.out.print("Maximum number to sieve: ");
}
int max = in.nextInt();
return max;
}

// Create boolean array of prime status of integers less than max
public static boolean[] sieve(int max) {

// Create initial array and assume all are primes
boolean[] arePrimes = new boolean[max];
for (int i = 0; i<max; i++) {
arePrimes[i] = true;
}

// Mark multiples of each prime as non-prime
for (int i = 2; i<max; i++) {
if (arePrimes[i]) {
for (int

Solution

Implementation

// Mark multiples of each prime as non-prime
for (int i = 2; i<max; i++) {
    if (arePrimes[i]) {
        for (int m = 2; i * m < max; m++) {
            arePrimes[i*m] = false;
        }
    }
}


Pretty close to the perfect implementation.

However...

Consider the Transitive relation of the multiplication operation: 5 times 2 is 10. 2 times 5 is 10. It doesn't matter in which order you multiply things.

So when you multiply, say, 3 by 2, you're just multiplying 2 by 3 again. And the same goes for 5 by 2, 7 by 2, 11 by 2... but also 5 by 3, 7 by 3, 7 by 5, etc.

What you'd do in this case is start m off at the value of i. This solves the issue where you're multiplying 2 with 3 and 3 with 2, thus checking the same thing twice.

There's another issue: Now that we're effectively checking (i i) < max in the inner loop on the first iteration, it doesn't make sense for the outer loop to check for on i < max. Instead, you'd want to check for (i i) < max there as well.

// Mark multiples of each prime as non-prime
for (int i = 2; i * i < max; i++) {
    if (arePrimes[i]) {
        for (int m = i; i * m < max; m++) {
            arePrimes[i*m] = false;
        }
    }
}


I was also a bit worried about getMax willing to accept negative numbers, but Java will throw a java.lang.NegativeArraySizeException, and as such, the error should be pretty clear. For use in a larger application, you might want to handle the exception, rather than the program forcibly exiting.

Documentation

You should use Javadoc style comments. Simple //comments before each function generally don't get picked up by IDE's. By making use of Javadoc, you can provide descriptions of what a method does and what the meaning of each parameter is. Like that, not only will other programmers easily be able to search for information in a method's documentation, it can also be automated - like an IDE showing a tooltip for each method parameter as you type them.

Code Snippets

// Mark multiples of each prime as non-prime
for (int i = 2; i<max; i++) {
    if (arePrimes[i]) {
        for (int m = 2; i * m < max; m++) {
            arePrimes[i*m] = false;
        }
    }
}
// Mark multiples of each prime as non-prime
for (int i = 2; i * i < max; i++) {
    if (arePrimes[i]) {
        for (int m = i; i * m < max; m++) {
            arePrimes[i*m] = false;
        }
    }
}

Context

StackExchange Code Review Q#135225, answer score: 4

Revisions (0)

No revisions yet.