1

Recently, I've been attempting to create a program that prints prime numbers until a user-specified integer is achieved, the program itself including a "PrimeCheck" class, a "PrimeSieve" class of sorts, and a "Main" class:

public class PrimeCheck {

    boolean result;

    public PrimeCheck() {
        result = true;
    }

    public boolean primeCheck (int num) {
        int i, num1 = num - 1;
        for (i = num1; i > 1; i--) {
            if (num % i == 0) {
                result = false;
            }
        }
        return result;
    }

}

import java.util.ArrayList;

public class PrimeSieve {

    public PrimeSieve() {

    }

    PrimeCheck PCObj = new PrimeCheck();
    ArrayList<Integer> primes = new ArrayList<Integer>();

    public void primeSieve(int num) {
        int[] arr = new int[num];
        for (int i = 0; i < num; i++) {
            arr[i] = i + 1;
            if (PCObj.primeCheck(arr[i]) == true) {
                primes.add(arr[i]);
            }
        }
        for (int c = 0; c < primes.size(); c++) {
            System.out.print(primes.get(c) + " ");
        }
    }

}

import java.util.Scanner;

public class PrimeSieveMain {
    public static void main(String[] args) {
        Scanner input = new Scanner(System.in);
        PrimeSieve PSObj = new PrimeSieve();
        System.out.println("Prime Sieve");
        System.out.print("Limit: ");
        int limit = input.nextInt();
        PSObj.primeSieve(limit);
    }
}

Pardon my inexperience, yet I cannot seem to locate the problem in this program.

RealSkeptic
  • 33,993
  • 7
  • 53
  • 79
Alexander Burka
  • 23
  • 2

3 Answers3

2

Your problem is in the PrimeCheck class. That class has a state variable (a field) named result. State variables retain the value between calls, for as long as the object is "alive".

So as soon as you hit a number that is not prime, you set this result to false. That value is kept and never changes.

The result variable should be a local variable, not a state variable, and it should be set to true at the beginning of the method. This way it will start fresh every time.

Other notes:

  • There is really no point in the PrimeCheck class. It doesn't represent a real "entity", and the method can easily be added to the PrimeSieve class. Creating classes for different entities is a good practice, but I think in this case there is no point - it just has one function and that function doesn't depend on anything but its parameters.
  • If you meant to represent the Sieve of Eratosthenes then this is not the correct algorithm. This is the naive algorithm - it just tests each number individually and doesn't cross out multiples of previous primes as the real Sieve does.
RealSkeptic
  • 33,993
  • 7
  • 53
  • 79
  • First, thank you. Second, this wasn't really supposed to be reflective of the Sieve of Eratosthenes. Being new to comp sci in general, I just wanted to create an original algorithm that could print primes until a specified ceiling – Alexander Burka Jun 30 '15 at 04:06
1

The PrimeCheck has serveral design problems, the first is you designed the result variable as a member, and its only initialized to true upon construction, but updated with false in primeCheck(). Once it has returned false, it will return false on all subsequent calls.

Its also not necessary to design the result as a member, since the result is only related to the method primeCheck(), thus change it to return the value directly, eliminating the member:

public class PrimeCheck {
    public boolean primeCheck (int num) {
        for (int i = num - 1; i > 1; i--) {
            if (num % i == 0) {
                return false;
            }
        }
        return true;
    }
}

Since PrimeCheck now has no state left, the method could also be made static, making the PrimeCheck instance in your program superflous. You could just call the static method.

PrimeCheck is also terribly inefficient, due to several design choices - one is you start testing from (num - 1), but the most common divisors are the smallest numbers. So it would be more efficient to start testing from the lower end and work the loop upwards. The upper bound (num - 1) is also chosen poorly. The possible largest divisor for num is the square root of num, so the upper bound should be that.

Durandal
  • 19,919
  • 4
  • 36
  • 70
  • Any other suggestions to improve the efficiency of the program? Again, I'm pretty new to Java and programming alike, so I've devoted the majority of my instruction to learning the language itself and and learning how to solve basic problems with the employment of computers, rather than focusing on the efficiency of the algorithm. – Alexander Burka Jun 30 '15 at 04:19
  • @AlexanderBurka Don't worry about the efficiency too much - your first priority should always be *correctness*, not speed. Major leaps in performance are always the result of improving the algorithm; or at least parts of it (e.g. eliminating superflous computations, or keeping the result of a costly computation for later reuse). As far as the languages go, there isn't really much to choose from; look at how small the number of keywords in java is. The basic computation/decision statements in every language are practically the same in terms of functionality (syntactic sugar aside). – Durandal Jun 30 '15 at 14:40
  • @AlexanderBurka (ran out of space) - So in conclusion its always the efficiency of the algorithm you should strive to improve on - that the place where there is usually the most bang for your effort/time to be gained. Optimizing code is your *last resort*, when you have exhausted you other options and still need to improve on speed. – Durandal Jun 30 '15 at 14:42
  • thanks for the tips. One question though, which doesn't really require a whole new thread: pertaining to the efficiency of algorithms in Java, in what instances should I be making a class and consequently creating an instance of that class vs simply declaring a static method? – Alexander Burka Jul 02 '15 at 00:34
  • @AlexanderBurka There are two aspects to static vs instance method, the latter can be overwritten (polymorphism) and it has automatic access to its other members - this comes at two expenses, first the object has to be created at some point, second instance method calls are potentially slightly more costly to execute because of their implicit "this" reference as well as resolving the correct method (potentially because the VM can optimize some of the cost away). Static methods are the equivalent of most other languages *functions*. – Durandal Jul 02 '15 at 14:56
  • @AlexanderBurka The decision between static and instance should be *design* driven, that is use what makes sense - if the method operates on an objects data, it makes more sense to use an instance method on that object (the cost of object creation and "this"-refrence are moot in this case, because if you substituted it with a static method both would still apply). In practice almost all methods do logically belong to an object instance, and thus static methods are a minority. Prime static candidates are function helper methods operating *solely* on their arguments, e.g. java.lang.Math. – Durandal Jul 02 '15 at 15:00
0

When you get the number is not a prime:

public boolean primeCheck (int num) {
    int i, num1 = num - 1;
    for (i = num1; i > 1; i--) {
        if (num % i == 0) {
            result = false;
        }
    }
    return result;
}

result become false, and never change, so I suggest this:

public boolean primeCheck (int num) {
    result=true;
    int i, num1 = num - 1;
    for (i = num1; i > 1; i--) {
        if (num % i == 0) {
            result = false;
        }
    }
    return result;
}

Before you start to determine prime, you should presume it is a prime
Not tested, just an idea

maskacovnik
  • 3,080
  • 5
  • 20
  • 26