Christoph Goller wrote:
Sorry for the delay. I was busy with other things and it took some
time to fix the problems that Doug mentioned.

No problem. Thanks so much for working on this patch. It looks really good, and solves a real problem. We can't demand that volunteers work on a schedule!


My only concerns now are aesthetic. You add segmentInfos as a protected field of IndexReader, which is a public class. This means it will appear in the javadoc. Yet this field is of type SegmentInfos, a package-private class, which will not be in the javadoc.

I try hard to make the javadoc only show things which are meant to be a part of the public API. So the question is, should SegmentInfos be a public class, i.e., do we think that folks might need to reference it in their application code, or should it remain package private?

My rule is that, if you cannot imagine a reasonable use for something, or if there's an already another public way of doing it, don't make it public. Keeping more of the implementation private makes it easier to compatibly evolve the implementation. If they really want, folks can change things in their private copy of the sources. What we want to avoid is incompatible changes for folks who only use public APIs. Over the long term, a smaller public API is easier to maintain.

My preference is that SegmentInfos remain package private. The easiest way to do this would just be to make the segmentInfos field of IndexReader package private (i.e. with no public/private declaration). The only problem is that then IndexReader would not be correctly subclassable from outside the package.

Alternately, segmentInfos could be made private, will all access from IndexReader only. Can you see a way to get things to work correctly this way?

Finally, you add a new constructor to SegmentsReader but the old constructor is no longer used (except by the new constructor). It would be better to simply replace the existing constructor with a new constructor. Less code is better.

If you are tired of working on this, and/or have no more time, please say so. If you are unable, I will try to address the above issues myself.

Thanks again for all your contributions,

Doug


--------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]



Reply via email to