Skip to main content
added 2 characters in body
Source Link
palacsint
  • 29.9k
  • 9
  • 81
  • 156
  1. You should indicate in the javadoc of the class that it's thread-safe.

  2. You should use a private lock object instead of synchronized methods:

     private final Object lock = new Object();
     ...
     public synchronized List<Integer> getPrimesUptoN(int n) {
         synchronized (lock) {
             validatePositive(n); 
             validateOutOfBound(n);
             ...
             return Collections.unmodifiableList(computePrimesUptoN(n));
         }
     }
    

Consider the following code:

    final PrimeUtil primeUtil = new PrimeUtil();
    synchronized (primeUtil) {
        
    }

Anyone can lock on this instance which could lead to poor performance or deadlock.

See also: Effective Java 2nd Edition, Item 70: Document thread safety

1.

private static final int DEFAULT_SIZE = 100;

I think DEFAULT_LOOK_AHEAD would be more descriptive. DEFAULT_SIZE is a little bit misleading.

  1. I'd rename the field to primeCache and remove the comment:
// cache of primes.
public final List<Integer> primes;

See Clean Code by Robert C. Martin, Don’t Use a Comment When You Can Use a Function or a Variable, p67

1.

primes = new ArrayList<Integer>();

You could save a line by putting this on the same line as the declaration:

    public final List<Integer> primes = new ArrayList<Integer>();
 

I think it would be a little bit easier to follow.

  1. I'd put the validate methods at the end of the file. The are not so important to be the first methods.

if (primeIndex != -1 && primeIndex != primes.size()) {

I'd create a NOT_FOUNDNOT_FOUND constant for -1 here for better readability.

  1. You should indicate in the javadoc of the class that it's thread-safe.

  2. You should use a private lock object instead of synchronized methods:

     private final Object lock = new Object();
     ...
     public synchronized List<Integer> getPrimesUptoN(int n) {
         synchronized (lock) {
             validatePositive(n); 
             validateOutOfBound(n);
             ...
             return Collections.unmodifiableList(computePrimesUptoN(n));
         }
     }
    

Consider the following code:

    final PrimeUtil primeUtil = new PrimeUtil();
    synchronized (primeUtil) {
        
    }

Anyone can lock on this instance which could lead to poor performance or deadlock.

See also: Effective Java 2nd Edition, Item 70: Document thread safety

1.

private static final int DEFAULT_SIZE = 100;

I think DEFAULT_LOOK_AHEAD would be more descriptive. DEFAULT_SIZE is a little bit misleading.

  1. I'd rename the field to primeCache and remove the comment:
// cache of primes.
public final List<Integer> primes;

See Clean Code by Robert C. Martin, Don’t Use a Comment When You Can Use a Function or a Variable, p67

1.

primes = new ArrayList<Integer>();

You could save a line by putting this on the same line as the declaration:

    public final List<Integer> primes = new ArrayList<Integer>();
 

I think it would be a little bit easier to follow.

  1. I'd put the validate methods at the end of the file. The are not so important to be the first methods.

if (primeIndex != -1 && primeIndex != primes.size()) {

I'd create a NOT_FOUND constant for -1 here for better readability.

  1. You should indicate in the javadoc of the class that it's thread-safe.

  2. You should use a private lock object instead of synchronized methods:

     private final Object lock = new Object();
     ...
     public synchronized List<Integer> getPrimesUptoN(int n) {
         synchronized (lock) {
             validatePositive(n); 
             validateOutOfBound(n);
             ...
             return Collections.unmodifiableList(computePrimesUptoN(n));
         }
     }
    

Consider the following code:

    final PrimeUtil primeUtil = new PrimeUtil();
    synchronized (primeUtil) {
        
    }

Anyone can lock on this instance which could lead to poor performance or deadlock.

See also: Effective Java 2nd Edition, Item 70: Document thread safety

1.

private static final int DEFAULT_SIZE = 100;

I think DEFAULT_LOOK_AHEAD would be more descriptive. DEFAULT_SIZE is a little bit misleading.

  1. I'd rename the field to primeCache and remove the comment:
// cache of primes.
public final List<Integer> primes;

See Clean Code by Robert C. Martin, Don’t Use a Comment When You Can Use a Function or a Variable, p67

1.

primes = new ArrayList<Integer>();

You could save a line by putting this on the same line as the declaration:

    public final List<Integer> primes = new ArrayList<Integer>();
 

I think it would be a little bit easier to follow.

  1. I'd put the validate methods at the end of the file. The are not so important to be the first methods.

if (primeIndex != -1 && primeIndex != primes.size()) {

I'd create a NOT_FOUND constant for -1 here for better readability.

Source Link
palacsint
  • 29.9k
  • 9
  • 81
  • 156

  1. You should indicate in the javadoc of the class that it's thread-safe.

  2. You should use a private lock object instead of synchronized methods:

     private final Object lock = new Object();
     ...
     public synchronized List<Integer> getPrimesUptoN(int n) {
         synchronized (lock) {
             validatePositive(n); 
             validateOutOfBound(n);
             ...
             return Collections.unmodifiableList(computePrimesUptoN(n));
         }
     }
    

Consider the following code:

    final PrimeUtil primeUtil = new PrimeUtil();
    synchronized (primeUtil) {
        
    }

Anyone can lock on this instance which could lead to poor performance or deadlock.

See also: Effective Java 2nd Edition, Item 70: Document thread safety

1.

private static final int DEFAULT_SIZE = 100;

I think DEFAULT_LOOK_AHEAD would be more descriptive. DEFAULT_SIZE is a little bit misleading.

  1. I'd rename the field to primeCache and remove the comment:
// cache of primes.
public final List<Integer> primes;

See Clean Code by Robert C. Martin, Don’t Use a Comment When You Can Use a Function or a Variable, p67

1.

primes = new ArrayList<Integer>();

You could save a line by putting this on the same line as the declaration:

    public final List<Integer> primes = new ArrayList<Integer>();
 

I think it would be a little bit easier to follow.

  1. I'd put the validate methods at the end of the file. The are not so important to be the first methods.

if (primeIndex != -1 && primeIndex != primes.size()) {

I'd create a NOT_FOUND constant for -1 here for better readability.