JSword
  1. JSword
  2. JS-274

VerseRange.contains(Key) VerseRange.contains(Verse) problem

    Details

    • Type: Bug Bug
    • Status: Open (View Workflow)
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: o.c.jsword.book
    • Labels:
      None

      Description

       private VerseRange gen_all = ...;
      
          public void testKeyContainsVerse() {
              // this passes
              assertTrue(gen_all.contains(gen11));
              
              // this fails
              Key gen_allKey = gen_all;
              assertTrue(gen_allKey.contains(gen11));
          }
      

      DM:
      I think that it is compile time determination of what is being called.

      In the gen_all case, it is known that it is a VerseRange and that VerseRange has a more specific contains(Verse). So that is called.

      In the gen_allKey case, it is no longer a VerseRange but a Key (which should not know anything about a Verse, since it is supposed to work for all kinds of Keys). So it cannot call contains(Verse) but only contains(Key).

      MD:
      There are specific contains() implementations in VerseRange.

          public boolean contains(Verse that) {
              return v11n.distance(start, that) >= 0 && v11n.distance(that, end) >= 0;
          }
      
          public boolean contains(Key key) {
              if (key instanceof VerseRange) {
                  return contains((VerseRange) key);
              }
              return false;
          }
      

      The second is being called but returns false because key is not a VerseRange. If the implementation of VerseRange.contains(Key) is the problem then another clause added at the end of the method might be one way to fix it, like:

          public boolean contains(Key key) {
              if (key instanceof VerseRange) {
                  return contains((VerseRange) key);
              }
              if (key instanceof Verse) {
                  return contains((Verse) key);
              }
              return false;
          }
      

      DM:
      I've been mulling over whether it is a sufficient solution. There are other "Key"s that hold verses (i.e. Passage and it ilk). A VerseRange can certainly contain all the Verses in a Passage. For example, a VerseRange of Genesis 1 contains a Passage of Gen 1:1-2 and Gen 1:10. I don't think this is at all common in real code, but it is possible.

        Activity

        Hide
        DM Smith added a comment -

        I thought I checked this in. Can't find it. Will do so soon. I haven't explored whether it is a sufficient solution.

        Show
        DM Smith added a comment - I thought I checked this in. Can't find it. Will do so soon. I haven't explored whether it is a sufficient solution.
        Hide
        Martin Denham added a comment - - edited

        Sorry, I just checked and it isn't fixed. I was confused too.

        The simple but possibly not comprehensive fix was a new 'instanceof Verse' to be added to VerseRange.containsKey(Key) as below:

        public boolean contains(Key key) {
                if (key instanceof VerseRange) {
                    return contains((VerseRange) key);
                }
                if (key instanceof Verse) {
                    return contains((Verse) key);
                }
                return false;
            }
        
        Show
        Martin Denham added a comment - - edited Sorry, I just checked and it isn't fixed. I was confused too. The simple but possibly not comprehensive fix was a new 'instanceof Verse' to be added to VerseRange.containsKey(Key) as below: public boolean contains(Key key) { if (key instanceof VerseRange) { return contains((VerseRange) key); } if (key instanceof Verse) { return contains((Verse) key); } return false ; }
        Hide
        DM Smith added a comment -

        Many thanks. The simple fix has been checked in. I'm leaving this open to track the more comprehensive solution.

        Show
        DM Smith added a comment - Many thanks. The simple fix has been checked in. I'm leaving this open to track the more comprehensive solution.

          People

          • Assignee:
            DM Smith
            Reporter:
            Martin Denham
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated: