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

Printing primes between user-given numbers

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

Problem

I want to display all the prime numbers between two integers taken from the user as input. Also I have to take a user input to specify the number of times the loop should run. Here is the Java code that I have written. I have to now reduce the run-time.

import java.util.Scanner;

    public class Main{
        private static Scanner x;

        public static void main(String[] args) {
            Scanner x = new Scanner(System.in);
            int num = x.nextInt();
            for (int t = 1; t = 1 && n <= 1000000000 && n - m <= 100000) {
                    for (int current = m; current <= n; current++) {
                        int sqr_root = (int) Math.sqrt(current);
                        boolean is_prime = true;
                        for (int i = 2; i <= sqr_root; i++) {
                            if (current % i == 0) {
                                is_prime = false;
                            }
                        }
                        if (is_prime) {
                            System.out.println(current);
                        }
                    }
                }
            }
        }
    }

Solution

Others have already said to change the algorithm.
There are other general coding practice problems that are worth highlighting too.

Naming

Most of the variable names are awful. If I see x, I'll never guess it's a Scanner.
m and n for the lower and upper limits are poor names too,
as it's easy to mix up which is which.
sqr_root and is_prime don't follow the Java convention of using camelCase.

Testing

How do you test that your solution actually works?
You rerun the program and enter some inputs and verify that the result is correct?
And if you make a change? You recompile, rerun, reverify?
That's an awful lot of work.
It's a lot less work to write some unit tests instead,
which you can easily re-run as simply as the click of a button,
and the result is as easy to verify as seeing all green lights or not.

However, to make the implementation unit testable, it needs to change a bit:

  • Make the method to take the inputs as parameters instead of standard input



  • Make the method return the list of primes found



Something like this:

private static final int MAX_UPPER = 1000000000;
private static final int MAX_DIFF = 100000;

public static void main(String[] args) {
    Scanner scanner = new Scanner(System.in);
    int num = scanner.nextInt();
    for (int t = 1; t  getPrimesBetween(int lower, int upper) {
    List primes = new ArrayList();
    if (lower >= 1 && upper <= MAX_UPPER && upper - lower <= MAX_DIFF) {
        for (int current = lower; current <= upper; current++) {
            int squareRoot = (int) Math.sqrt(current);
            boolean isPrime = true;
            for (int i = 2; i <= squareRoot; i++) {
                if (current % i == 0) {
                    isPrime = false;
                    break;
                }
            }
            if (isPrime) {
                primes.add(current);
            }
        }
    }
    return primes;
}


And some unit tests, to get you started:

@Test
public void testBetween_4_and_5() {
    assertEquals(Arrays.asList(5), getPrimesBetween(4, 5));
}

@Test
public void testBetween_4_and_9() {
    assertEquals(Arrays.asList(5, 7), getPrimesBetween(4, 9));
}


Extract constants

Notice in the example earlier,
I converted your original hard-coded constants to proper constants.

private static final int MAX_UPPER = 1000000000;
private static final int MAX_DIFF = 100000;


It's cleaner and better this way.

Correctness

It's not so good that the method returns an empty list if you use a too high input number or too big difference, without any indication that something went wrong.
The user might expect a huge list of numbers between 1 and 99999999, or an error, but definitely not an empty list.

Here's a unit test that fails now, until you fix the method to throw an exception in the case of unsupported input values.

@Test(expected = IllegalArgumentException.class)
public void testTooBigDiff() {
    int lower = 11;
    int upper = lower + MAX_DIFF + 1;
    getPrimesBetween(lower, upper);
}

Code Snippets

private static final int MAX_UPPER = 1000000000;
private static final int MAX_DIFF = 100000;

public static void main(String[] args) {
    Scanner scanner = new Scanner(System.in);
    int num = scanner.nextInt();
    for (int t = 1; t <= num; t++) {
        int lower = scanner.nextInt();
        int upper = scanner.nextInt();

        for (Integer prime : getPrimesBetween(lower, upper)) {
            System.out.println(prime);
        }
    }
}

static List<Integer> getPrimesBetween(int lower, int upper) {
    List<Integer> primes = new ArrayList<Integer>();
    if (lower >= 1 && upper <= MAX_UPPER && upper - lower <= MAX_DIFF) {
        for (int current = lower; current <= upper; current++) {
            int squareRoot = (int) Math.sqrt(current);
            boolean isPrime = true;
            for (int i = 2; i <= squareRoot; i++) {
                if (current % i == 0) {
                    isPrime = false;
                    break;
                }
            }
            if (isPrime) {
                primes.add(current);
            }
        }
    }
    return primes;
}
@Test
public void testBetween_4_and_5() {
    assertEquals(Arrays.asList(5), getPrimesBetween(4, 5));
}

@Test
public void testBetween_4_and_9() {
    assertEquals(Arrays.asList(5, 7), getPrimesBetween(4, 9));
}
private static final int MAX_UPPER = 1000000000;
private static final int MAX_DIFF = 100000;
@Test(expected = IllegalArgumentException.class)
public void testTooBigDiff() {
    int lower = 11;
    int upper = lower + MAX_DIFF + 1;
    getPrimesBetween(lower, upper);
}

Context

StackExchange Code Review Q#66498, answer score: 6

Revisions (0)

No revisions yet.