aherbert commented on pull request #258: URL: https://github.com/apache/commons-collections/pull/258#issuecomment-1037596846
Thanks for the updates. Currently the CI build does not check code coverage, so you have to do this locally. Can you look at test coverage again: ``` mvn test jacoco:report open target/site/jacoco/org.apache.commons.collections4.bloomfilter/index.html ``` You have some code that is picked up by missing coverage. Looking at the code some of these cases really should be covered and relate to operations performed on filters of different sizes. A few points: - The code at SparseBloomFilter Line 221 is just weird; it looks like testing code. - Some constructors in the BloomFilters are not used at all. - The SimpleBloomFilter is catching IOOB exception and rethrowing it. Is this necessary? I would let the IOOB exception trickle up. If you want to catch it then at least test this works and raises an IAE rather than a IOOBE. - In SimpleBloomFilter.forEachBitMap is the bitMap ever null? - No coverage that the `makePredicate` in BitMapProducer will send `0L` to the LongBiFunction when the array is exhausted. Note the documentation for makePredicate uses `apply` when it should be using `forEachBitMap`. This needs a better example that can actually be compiled. The method should better explain the usage: "Create a single long argument predicate to be passed to the `forEachMethod` of a second BitMapProducer. For each `long` passed to the predicate from the second BitMapProducer a matching `long` from this BitMapProducer will be paired with it and passed to the argument LongBiFunction. The `long` from this producer will be the first argument to the BiFunction; when the bitmaps are exhausted from this producer then the value 0 is substituted until the second producer is also exhausted". In this use case the LongBiFunction should be named a LongBinaryPredicate as it returns a boolean if using the `java.util.function` naming conventions. So there's a few things to fix up and I'll do a full review of what we now have in the coming few days. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
