hapifhir / hapi-hl7v2

277 stars 138 forks source link

DatumPath.numbersLessThan Bug? 🪲 #84

Open milkshakeuk opened 2 years ago

milkshakeuk commented 2 years ago

Hi @ohr, @jamesagnew

I have been looking to port some more hapi to nHapi and as I was porting DatumPath.numbersLessThan I started to write unit tests (which do not exist in hapi) and discovered that both the following are true but this shouldn't be the case should it?

How can both these statements be true?

[1, 1, 5, 5] < [1, 2]
[1, 2] < [1, 1, 5, 5]
milkshakeuk commented 2 years ago

@ohr, @jamesagnew

I've done some more comparisons this time using IKVM to load the hapi-base-2.3.jar into my dotnet unit test project so I could compare the output and here are the results.

Looks like I haven't made a mistake in the porting process as they both return the same results give then example above.

image

image

milkshakeuk commented 2 years ago

@jamesagnew @ohr

I believe this will fix it, I have made the change in the nhapi port and it seems to fix it (proven with unit tests), assuming that's the expected behaviour and I am not misunderstanding it.

public boolean numbersLessThan(DatumPath other)
{
    DatumPath extendedCopyThis = new DatumPath(this);
    extendedCopyThis.setSize(s_maxSize);

    DatumPath extendedCopyOther = new DatumPath(other);
    extendedCopyOther.setSize(s_maxSize);

    boolean lessThan = false;
    for(int i=1; !lessThan && (i<s_maxSize); ++i) {
        int this_i = ((Integer)extendedCopyThis.get(i));
        int other_i = ((Integer)extendedCopyOther.get(i));
        // this following line is the fix
        // if this_i is greater than other_i then just stop iterating as the answer is clearly false.
        if(this_i > other_i) break;
        lessThan = (this_i < other_i);
    }

    return lessThan;
}

I would create a PR however I have yet been successful in actually building hapi never mind running the test suite.

milkshakeuk commented 10 months ago

@jamesagnew did you see this?