patternjavaMinor
BitSet wrapper to act as a sieve abstraction
Viewed 0 times
actabstractionsievewrapperbitset
Problem
This is a
Test Cases
```
package info.simpll.immense.prime;
import org.junit.Test;
import java.math.BigInteger;
import static org.junit.Assert.*;
/**
* @author Bhathiya
*/
public class SieveTest {
@Test
public void testApi() {
Sieve instance = new Sieve(10, 2);
boolean result = instance.get(10);
instance.debugPrint("testApi");
assertFalse(result);
assertTrue(instance.getMaxIndex()
.compareTo(BigInteger.valueOf(11)) == 0);
}
@Test
public void testSetAndGet() {
Sieve instance = new Sieve(10, 2);
for (int i = 2; i < (int) instance
BitSet wrapper class to act as a Sieve abstraction for a prime calculator. Review for performance and Java/Java 8/Guava best practices.package info.simpll.immense.prime;
import com.google.common.base.MoreObjects;
import java.math.BigInteger;
import java.util.BitSet;
import java.util.Objects;
import java.util.stream.IntStream;
/**
* Sieve holds a BitSet that is used to cross out numbers,
* However unlike BitSet, sieve uses different indexing scheme
* Which is more suitable for prime number calculation
*
* @author Bhathiya
*/
public class Sieve {
public static int MAX_SIZE = 8 * 1024 * 1024;
private final int size;
private BigInteger startIndex;
private BigInteger endIndex;
private final BitSet bitSet;
/**
* Constructor for the bitSet
*
* @param size count of the bitSet
* @param startIndex starting index of the bitSet
*/
public Sieve(int size, BigInteger startIndex) {
if (size > MAX_SIZE || size {
BigInteger index = startIndex.add(BigInteger.valueOf(i));
if (!get(index)) {
probablePrimeList.append(" ");
probablePrimeList.append(index.toString(10));
}
});
System.out.printf("SIEVE: %s %s: Unset {%s }\n",
prepend, this.toString(), probablePrimeList);
}
}Test Cases
```
package info.simpll.immense.prime;
import org.junit.Test;
import java.math.BigInteger;
import static org.junit.Assert.*;
/**
* @author Bhathiya
*/
public class SieveTest {
@Test
public void testApi() {
Sieve instance = new Sieve(10, 2);
boolean result = instance.get(10);
instance.debugPrint("testApi");
assertFalse(result);
assertTrue(instance.getMaxIndex()
.compareTo(BigInteger.valueOf(11)) == 0);
}
@Test
public void testSetAndGet() {
Sieve instance = new Sieve(10, 2);
for (int i = 2; i < (int) instance
Solution
A few things that jump to my eye, here and there:
this will come out somewhat... strange to me as a developer using your class... Why can't I sieve for things smaller than 10?? And what is the allowed maximum size?
When this gets thrown to me, I may know what I did wrong, but the upper bound is just found by trial and error. Be more descriptive. (and if you really need to keep it, extract 10 into a named constant)
Same problem applies here. Usually I'd expect to be told two things when there's an
Both of these are missing here. These messages need some work...
debug... print..These two words are too often placed together in the same sentence. Debugging is not randomly printing out variables and object internals with some strings in front. Debugging is stepping through code with a debugger.
Everything else is info-logging. and that should probably not happen in a class like this. This method is a problem, in that it's effectively useless for the functionality of the class. It doesn't belong there, move it somewhere else.
Last but not least: Test setup.
Unit tests in my sideprojects usually have a standard setup. There's this one thing getting tested. Simplest name: "class under test" or short
Before every test, this class under test will be put into a clean state, so the test doesn't have to concern with that. JUnit provides a nice annotation for that: @Before
I'd change your unit tests to look a little more like this:
if (size > MAX_SIZE || size < 10) {
throw new IllegalArgumentException("Size is larger than "
+ "the allowed maximum or smaller than 10");
}this will come out somewhat... strange to me as a developer using your class... Why can't I sieve for things smaller than 10?? And what is the allowed maximum size?
When this gets thrown to me, I may know what I did wrong, but the upper bound is just found by trial and error. Be more descriptive. (and if you really need to keep it, extract 10 into a named constant)
if (getEndIndex().compareTo(index) <= 0
|| getStartIndex().compareTo(index) == 1) {
throw new IndexOutOfBoundsException("Invalid position");
}Same problem applies here. Usually I'd expect to be told two things when there's an
IndexOutOfBoundsException (and yes, in the exception message)- What index did I try to access?
- What size does the Collection I try to access have?
Both of these are missing here. These messages need some work...
public void debugPrint(String prepend) {
StringBuilder probablePrimeList = new StringBuilder();
IntStream.rangeClosed(0, size - 1).forEach(i -> {
BigInteger index = startIndex.add(BigInteger.valueOf(i));
if (!get(index)) {
probablePrimeList.append(" ");
probablePrimeList.append(index.toString(10));
}
});
System.out.printf("SIEVE: %s %s: Unset {%s }\n",
prepend, this.toString(), probablePrimeList);
}debug... print..These two words are too often placed together in the same sentence. Debugging is not randomly printing out variables and object internals with some strings in front. Debugging is stepping through code with a debugger.
Everything else is info-logging. and that should probably not happen in a class like this. This method is a problem, in that it's effectively useless for the functionality of the class. It doesn't belong there, move it somewhere else.
Last but not least: Test setup.
Unit tests in my sideprojects usually have a standard setup. There's this one thing getting tested. Simplest name: "class under test" or short
cut.Before every test, this class under test will be put into a clean state, so the test doesn't have to concern with that. JUnit provides a nice annotation for that: @Before
I'd change your unit tests to look a little more like this:
public class SieveTest {
private Sieve cut;
@Before
public void setup() {
cut = new Sieve(10,2);
}
// test cases go here, access cut
}Code Snippets
if (size > MAX_SIZE || size < 10) {
throw new IllegalArgumentException("Size is larger than "
+ "the allowed maximum or smaller than 10");
}if (getEndIndex().compareTo(index) <= 0
|| getStartIndex().compareTo(index) == 1) {
throw new IndexOutOfBoundsException("Invalid position");
}public void debugPrint(String prepend) {
StringBuilder probablePrimeList = new StringBuilder();
IntStream.rangeClosed(0, size - 1).forEach(i -> {
BigInteger index = startIndex.add(BigInteger.valueOf(i));
if (!get(index)) {
probablePrimeList.append(" ");
probablePrimeList.append(index.toString(10));
}
});
System.out.printf("SIEVE: %s %s: Unset {%s }\n",
prepend, this.toString(), probablePrimeList);
}public class SieveTest {
private Sieve cut;
@Before
public void setup() {
cut = new Sieve(10,2);
}
// test cases go here, access cut
}Context
StackExchange Code Review Q#99036, answer score: 3
Revisions (0)
No revisions yet.