FireFox2000000 / Moonscraper-Chart-Editor

BSD 3-Clause "New" or "Revised" License
225 stars 61 forks source link

`GetNextNonInclusive` returns incorrect value under boundary conditions #101

Closed emptierset closed 1 year ago

emptierset commented 1 year ago

This almost certainly also applies to GetPreviousNonInclusive, but I did not test it.

Scenario: GetNextNonInclusive is called with songObjects=[BPM(position=0), BPM(position=1000)] and position=1000. Expected behavior: GetNextNonInclusive returns null. Observed behavior: GetNextNonInclusive returns BPM(position=1000).

I have reproduced the function's code below for convenience:

public static T GetNextNonInclusive<T>(IList<T> songObjects, uint position) where T : SongObject
{
    int pos = GetIndexOfNext(songObjects, position);
    if (pos != NOTFOUND)
    {
        if (songObjects[pos].tick == position && pos < songObjects.Count - 1)
            ++pos;

        return songObjects[pos];
    }
    else
        return null
}

The following code block demonstrates the correct behavior:

public static T GetNextNonInclusive<T>(IList<T> songObjects, uint position) where T : SongObject
{
    int pos = GetIndexOfNext(songObjects, position);
    if (pos != NOTFOUND)
    {
        if (songObjects[pos].tick == position)
        {
            if (pos >= songObjects.Count - 1)
                return null
            ++pos;
        }

        return songObjects[pos];
    }
    else
        return null
}

(and I'd probably rewrite it like this, given the chance):

public static T GetNextNonInclusive<T>(IList<T> songObjects, uint position) where T : SongObject
{
    int pos = GetIndexOfNext(songObjects, position);
    if (pos == NOTFOUND)
        return null;

    if (songObjects[pos].tick == position)
    {
        ++pos;
        if (pos >= songObjects.Count)
            return null;
    }

    return songObjects[pos];
}
FireFox2000000 commented 1 year ago

Is there any actual functionality issue that is resulting from this behaviour?

emptierset commented 1 year ago

No idea. I ran into it working on my personal fork of the program. It's just obviously incorrect behavior, so there may be some logic that assumes this function works as it claims to.

FireFox2000000 commented 1 year ago

Won't fix, no explicit functionality issue.