commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Claude Warren <cla...@xenei.com>
Subject Re: New Sub-project Proposal.
Date Mon, 23 Sep 2019 10:59:15 GMT
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 <claude@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 <gilleseran@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-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@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

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message