I will rework to remove the package private and other access issues noted. In Builder there is a difference between with() and build(). This follows the pattern established by MessageDigest[1] where it is possible to build a digest in one call or by adding multiple items and then calling digest. The differences here are digest has an update() method the builder uses with(), digest uses a digest() method to return the result builder uses build().
The serializable classes are classes that in the indexing code are sent across the wire or stored. I suppose there are other ways to do this and the serialization could be removed. Not a direction I would like to take but is doable. HammingWeight and Log values re used in indexing. I think HammingWeight is used in the collections as well. HammingWeight is a well known property of a bloom filter. The Log not so much so. I suppose the log could be removed and created as a separate library method that would execute against BitSets. Moving the calculation outside of the Bloom filter means that we will have to make copies of bloom filters to perform the log and the log will not be retained with the filter when used in indexing. Murmer 128 hash is not in Commons Codec (at least I could not find it). Perhaps it should be added? I could extract the code and provide it there if that is the appropriate solution. ProtoBloomFilter is not an abbreviation of PrototypeBloomFilter as it is not a Bloom filter but a primitive object from which Bloom filters may be created. I think this is a proper use of the Proto[2] prefix. Package name plural. I thought there was some disagreement on this, however I will change it. I will revert the FindBugs config change as it is now commented out anyway. How does one suppress the fall through checkstyle error. I could find no annotations that would suppress the error and did not want to add another dependency to the build. I did find that other components in the project are using the checkstyle filter and so added the supression of the FallThrough check. I The reference to what is a Bloom filter is in bloomfilters/package-info.java. Does there need to be a reference from the BloomFilter interface definition to that? Thank you for taking the time to review and comment. Claude [1] https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/security/MessageDigest.html [2] https://www.dictionary.com/browse/proto?s=t On Mon, Sep 23, 2019 at 11:13 AM Claude Warren <cla...@xenei.com> wrote: > For the style issues.... is there an Eclipse style package that meets the > commons style or some other tool that will correctly configure the format > and style options in Eclipse? > > On Mon, Sep 23, 2019 at 10:54 AM Gilles Sadowski <gillese...@gmail.com> > wrote: > >> Hello. >> >> Here are a few comment from a quick browse of today's update >> of PR #83. >> >> * "package private for testing" is not a good reason (IMO) >> * There are spurious blank spaces in some of the file ("git diff" >> shows them in red) >> * You should always perform "git rebase master" >> * In "Builder": >> ** Field "hashes" should be "final" >> ** I still fail to see the necessity to duplicate "with" functionality >> with "build" methods >> * Why should so many classes be "Serializable"? >> * "StandardBloomFilter" is documented as "immutable" but it is not: >> There are 2 non "final" fields and 1 "final" but "protected" (hence >> it can be modified outside the class without ensuring its invariants) >> * IMHO use of transient fields is detrimental to the robustness of the >> design: It seems that "hamming weight" is not _used_ by the >> implementations (called only in the unit tests?) >> * Ditto for "logValue" >> * Did you check whether "murmur 128 hash" is available in "Commons >> Codec"? >> * Name of classes should not be abbreviated (cf. "Config", "Proto", >> "Stats" ...) >> * Package name is still plural: Should be singular IMO >> * Rule checks general suppression should be removed in the config >> files (CheckStyle, FindBugs) and replace by specific (within code) >> statements >> if applicable (e.g. the disabling of "no default in switch" is unnecessary >> since AFAICT a "default" is always provided). >> * All (also "private") fields and methods must be commented with Javadoc, >> not just code comment. >> * There should be a reference to what is a Bloom filter >> * Occurrences of "bloom" should be replaced with "Bloom" >> * There some style issues: >> ** There should be no space after an opening parenthesis, or before >> a closing one >> ** Curly braces position: opening on the same line, closing on a >> separate line >> >> Best regards, >> Gilles >> >> --------------------------------------------------------------------- >> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >> For additional commands, e-mail: dev-h...@commons.apache.org >> >> > > -- > I like: Like Like - The likeliest place on the web > <http://like-like.xenei.com> > LinkedIn: http://www.linkedin.com/in/claudewarren > -- I like: Like Like - The likeliest place on the web <http://like-like.xenei.com> LinkedIn: http://www.linkedin.com/in/claudewarren
