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

BitSet wrapper to act as a sieve abstraction

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

Problem

This is a 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:

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.