Hi all:
I've been participating in the user list for some time, and I'd like
to start helping maintain/enhance the code. So I thought I'd start
with something small, mostly to get the process down. Unit tests
sure fit the bill it seems to me, less chance of introducing errors
through ignorance but a fine way to extend *my* understanding
of Lucene.
I managed to check out the code and run the unit tests, which
was amazingly easy. I even managed to get the project into
IntelliJ and connect the codestyle.xml file. Kudos for whoever
set up the checkout/build process, I was dreading spending
days setting this up, fortunately I didn't have to.
So I, with Chris's help, found the code coverage report and
chose something pretty straightforward to test, BitUtil since it
was nice and self-contained. As I said, I'm looking at understanding
the process rather than adding much value the first time.
Alas, even something as simple as BitUtil generates questions
that I'm asking mostly to understand what approach the veterans
prefer. I'll argue with y'all next year sometime <G>.
So, according to the coverage report, there are two methods that
are never executed by the unit tests (actually 4, 2 that operate on
ints and 2 that operate on longs), isPowerOfTwo and
nextHighestPowerOfTwo. nextHighestPowerOfTwo is especially
clever, had to get out a paper and pencil to really understand it.
Issues:
1> none of these methods is ever called. I commented them out
and ran all the unit tests and all is well. Additionally, commenting
out one of the other methods produces compile-time errors so I'm
fairly sure I didn't do something completely stupid that just *looked*
like it was OK. I grepped recursively and they're nowhere in the
*.java files.
1a> What's the consensus about unused code? Take it out (my
preference) along with leaving a comment on where it can
be found (since it *is* clever code)? Leave it in because someone
found some pretty neat algorithms that we may need sometime?
1b> I'm not entirely sure about the contrib area, but the contrib jars
are all new so I assume "ant clean test" compiles them as well.
2> I don't agree with the behavior of nextHighestPowerOfTwo. Should
I make changes if we decide to keep it?
2a> Why should it return the parameter passed in when it happens to be
a perfect power of two? e.g. this passes:
assertEquals(BitUtil.nextHighestPowerOfTwo(128L), 128);
I'd expect this to actually return 256, given the name.
2b> Why should it ever return 0? There's no power of two that is
zero. e.g. this passes:
assertEquals(BitUtil.nextHighestPowerOfTwo(-1), 0);
as does this: assertEquals(BitUtil.nextHighestPowerOfTwo(0), 0).
*Assuming* that someone wants to use this sometime to, say, size
an array they'd have to test against a return of 0.
I'm fully aware that these are trivial issues in the grand scheme of things,
and I *really* don't want to waste much time hashing them over. I'll provide
a patch either way and go on to something slightly more complicated for
my next trick.
Best
Erick
