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
