> > [...] > > Gilles, > > Take a look at the PR, I added comments to allow the PR to have 0 deps. > > Gary > >
IMO, the package should be named "bloomfilter" (without "s"). Naming seems inconsistent in [Collections]: Some (package) names are singular, other plural. * Indent must be 4 spaces. * All fields and methods must be commented. * "FilterConfig" contains a "main" method. * Local variables and fields must be declared "final" whenever constant. * Some methods are declared "final", others not. * "BloomFilter" should be made immutable. * "BloomFilter" could do without a second constructor: "FilterConfig" should have a method that returns a "BitSet". * "BloomFilter" must make a defensive copy of its "bitSet" argument. * What's the difference between "match" and "equals"? * Why is "STORED_SIZE" public? * "Utils" classes should be avoided. * Hash function should be imported/shaded from "Commons Codec" (and defined there if missing). * Constructor(s) should be private (instantiation through factory methods (cf. ValJO). * "Serializable" should be avoided. * The "update" methods are missing explanations (or reference) about their purpose. Also, it seems that "update" and "build" are redundant. * Does "getLog" return a standard value? If so, the reference should appear in the Javadoc (not just as code comment). * What is the purpose of method "asBitSet()"? * (IIUC) "ProtoBloomFilter" should be renamed "BloomFilterFactory". "ProtoBloomFilterBuilder" should be a static inner class of that factory. Regards, Gilles --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
