[jsword-devel] Compare method in JSword

[DM Smith]

See embedded comments.
On Sep 5, 2012, at 11:45 AM, Chris Burrell <chris at burrell.me.uk> wrote:

> On the below, I was tempted, to fix this by using the distance() on the versification. However, that does not seem to work. Not sure why. Here was my code for VerseRange constructor:
> 
>         int distance = v11n.distance(start, end);

compareTo is not v11n safe. It should be marked as deprecated. Or a verse needs to have a v11n. I've been trying to avoid that and have a verse be a dumb holder of a Book, chapter number and a verse number.

Using v11n.distance(start, end) is the replacement for compareTo.

This code looks correct.

>         if(distance < 0) {
>             this.start = end;
>             this.end = start;
>             this.verseCount = calcVerseCount();
>         } else if (distance == 0) {
>             this.start = start;
>             this.end = start;
>             this.verseCount = 1;
>         } else {
>             this.start = end;
>             this.end = start;
>             this.verseCount = calcVerseCount();
>         }
> 
> Loads of tests then start failing.

The test might be wrong. Or only right for the KJV versification.


> Clearly, that's not quite replacing the constructor, like for like.

Looking at it, it appears that the problem is in the bowels of v11n.getOrdinal(verse).

Here is the code:
    public int getOrdinal(Verse verse) {
        return chapterStarts[bookList.getOrdinal(verse.getBook())][verse.getChapter()] + verse.getVerse();
    }

Obviously, the index for the book and the chapter have to be in bounds. One of them isn't. This method should not throw an AIOBE.

Looking at bookList.getOrdinal, it will return -1 if the book is not in the versification.
bookList.contains(book) will determine whether the book is in the versification or not.

If the book is not in the versification, there's not much that can be done. We cannot figure out whether to pick the first or last book in a testament. In fact, we cannot know which testament in which the book belongs.

verse.getChapter() is a bit simpler. It merely returns the chapter number contained in the verse. It should always be >=0. But it might be too big.

We have code to patch up such an out of range reference. See v11n.patch(Book, chapter, verse). (Probably the argument list should just be Verse) Basic idea: When the number was bigger than the book, we kept going into the next book. Likewise for the verse number.

We thought that the results were very surprising to an end user. The user makes a mistake or asks for something that isn't valid and is given something else without warning. So it is currently not used.

Note that the way that the verse is added in. It doesn't check to see whether the reference is in bounds of the chapter, let alone the v11n. So in a way, it "patches."

In v11n, there is also a validate method that will throw an NoSuchVerseException if the parts don't make sense for the v11n. 

Both validate and patch don't properly handle a book not in the versification. (i.e. bug in there.)

So, getOrdinal(verse) needs to be changed. Perhaps, wrap w/ try catching AIOBE and then at least logging the error and returning something marginally reasonable.

In the past we used to throw a NoSuchVerseException via validate when constructing a verse. Now that a verse does not have a v11n context that was removed.

In Him,
	DM


> 
> Chris
> 
> 
> On 5 September 2012 16:07, Chris Burrell <chris at burrell.me.uk> wrote:
> Hi
> 
> While I'm trying to retrieve a passage it seems to want to compare some keys. I'm looking at the DRC module, with Tob 1, but the problem occurs elsewhere.
> 
> Here's the stack trace I get. I'm at a lost to why we need to compare verse numbers here, but perhaps someone can enlighten me. Happy to fix it if I can understand what it's trying to do and a way forward.
> 
> Also,  I've made a small fix to the Versification elsewhere in the code (pull request in Git).
> 
> Caused by: java.lang.ArrayIndexOutOfBoundsException: -1
> 	at org.crosswire.jsword.versification.Versification.getOrdinal(Versification.java:503)
> 	at org.crosswire.jsword.passage.Verse.getOrdinal(Verse.java:440)
> 	at org.crosswire.jsword.passage.Verse.compareTo(Verse.java:261)
> 	at org.crosswire.jsword.passage.VerseRange.<init>(VerseRange.java:136)
> 	at org.crosswire.jsword.passage.VerseRangeFactory.fromText(VerseRangeFactory.java:124)
> 	at org.crosswire.jsword.passage.VerseRangeFactory.fromString(VerseRangeFactory.java:96)
> 	at org.crosswire.jsword.passage.VerseRangeFactory.fromString(VerseRangeFactory.java:61)
> 	at org.crosswire.jsword.passage.AbstractPassage.addVerses(AbstractPassage.java:879)
> 	at org.crosswire.jsword.passage.BitwisePassage.<init>(BitwisePassage.java:88)
> 	at org.crosswire.jsword.passage.RocketPassage.<init>(RocketPassage.java:70)
> 	at org.crosswire.jsword.passage.PassageType$1.createPassage(PassageType.java:44)
> 	at org.crosswire.jsword.passage.PassageKeyFactory.getKey(PassageKeyFactory.java:83)
> 	at org.crosswire.jsword.book.basic.AbstractPassageBook.getKey(AbstractPassageBook.java:217)
> 	at com.tyndalehouse.step.core.service.jsword.impl.JSwordPassageServiceImpl.getBookData(JSwordPassageServiceImpl.java:447)
> 	at com.tyndalehouse.step.core.service.jsword.impl.JSwordPassageServiceImpl.getOsisText(JSwordPassageServiceImpl.java:433)
> 	at com.tyndalehouse.step.core.service.impl.BibleInformationServiceImpl.getPassageText(BibleInformationServiceImpl.java:128)
> 	at com.tyndalehouse.step.rest.controllers.BibleController.getBibleText(BibleController.java:157)
> 	at com.tyndalehouse.step.rest.controllers.BibleController.getBibleText(BibleController.java:138)
> 	... 30 more
> 
> 
> Crhis
> 
> 
> _______________________________________________
> jsword-devel mailing list
> jsword-devel at crosswire.org
> http://www.crosswire.org/mailman/listinfo/jsword-devel

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.crosswire.org/pipermail/jsword-devel/attachments/20120905/b4dcd95d/attachment.html>