[jsword-devel] Patch for misc improvements

[DM Smith]

Stuff deleted. Except for requests for comments.

Joe Walker wrote:
> DM Smith wrote:
> 
>> 5) I changed:
>>         Key key = index.findWord(null);
>> to:
>>         Key key = PassageKeyFactory.instance().createEmptyKeyList();
>> The former would throw a NPE if index were null. Which was the case in 
>> the test code. It turns out that this was merely a way to create an 
>> empty key list. So the change makes the code clearer.
> 
> 
> It's not just PKF that I'm not keen on. I see Keys as depending on the 
> Book that created them.
This will become more true when we get to alternate versification. And 
other types of keys (e.g. Josephus has a couple of different 
hierarchical "versifications").

Thought for the future: It seems to me that a Book should have a way to 
indicate its hierarchical arrangement and a way of discovering how to 
optimally get at its data (like BookInfo does for modern English, 
canonical bibles). If we had this then all books could be treated 
uniformly and we could use verse as an address into a hierarchical space.

> So really we don't need a KeyFactory - we just 
> need a Book (which inherits from KF anyway).
> The advantage of the index.findWord(null); style is that it gets the 
> empty Key from the book and does not make the assumption that the Keys 
> are Passages. I think a search engine indexing a non-Bible could break 
> as a result of this. If you don't mind I'm not going to commit this 
> until we've debated it a bit more.

I did this because of a test that was failing. I figure that the JUnit 
tests are a way of stating expectations of the code and that behavior 
that they test should be preserved unless there is a deliberate 
migration from that behavior. Otherwise, what good are they?

Here is a fragment of the test code that I was trying to preserve:
// We shouldn't need a SearchableBible here because all of these should
// fail before any searching is done.
LocalParser engine = new LocalParser();
engine.init(null);

The specific problem is that it is passing a null index. Then in the 
search mechanism, it trys to build up a list of keys, but it gets the 
list with:
index.findWord(null)
which fails w/ a NPE.
I think that it was trying to avoid the overhead of creating an index. I 
can change the test so it creates and passes an index.

Since index.findWord(null) was not a good way, I changed it and got the 
test to work.

I think that at this time it would be best to make the changes from 
index.findWord(null) and tag them with a TODO, FIXME, BUG, ... and a 
note as to what the change should be.

The reason I think it would be best, is that I found some bugs in 
getting the JUnit test back up and running.

I will be glad to fix this.

> 
> But the index.findWord(null) idiom is very poor as you note. I guess we 
> need to add a method to index to create an empty Key. So I'll add a bug.
> 
> An on a similar vein, I wonder how many times KeyList crops up in 
> methods that ought to be Key.
> 
Perhaps all. After all, a Key is a key list.