bhlangonijr / chesslib

chess library for legal move generation, FEN/PGN parsing and more
Apache License 2.0
225 stars 78 forks source link

possible bug in MoveList.toSanArray() and toFanArray() #6

Closed evgentset closed 4 years ago

evgentset commented 6 years ago

Hi once again)

After I used MoveList.remove(Move) method, MoveList.toSanArray() returns incorrect value, because 'dirty' variable wasn't changed false by default and 'sanArray' is not nul (as I have used toSanArray() before ).

    public String[] toSanArray() throws MoveConversionException {
        if (!dirty && sanArray != null) {
            return sanArray;
        }
       ..//other code
        dirty = false;
        return sanArray;
    }

So, possible solutions are :

  1. override other methods from LinkedList where dataset is changing and set 'dirty = true'
  2. remove ' if (!dirty && sanArray != null)' and always recreate array from dataset
  3. make method 'setDitry(boolean)' - and user can call it before any other methods, like 'remove' etc.

Also I propose to make variables 'protected' instead of 'private' if variable don't have getters/setters. Then users of this library can get access to variables through inheritance.

Thanks, Eugene

bhlangonijr commented 6 years ago

Hi Eugene!

Thank you for your contributions!

Maybe the best option would be composing and not inheriting from LinkedList. But as it stands now I would prefer going with your #1option. Will do the change and take your suggestion on the protected methods.

Regards, Ben

On Wed, Aug 1, 2018 at 2:14 PM, Evgen notifications@github.com wrote:

Hi once again)

After I used MoveList.remove(Move) method, MoveList.toSanArray() returns incorrect value, because 'dirty' variable wasn't changed false by default and 'sanArray' is not nul (as I have used toSanArray() before ).

public String[] toSanArray() throws MoveConversionException {
    if (!dirty && sanArray != null) {
        return sanArray;
    }
   ..//other code
    dirty = false;
    return sanArray;
}

So, possible solutions are :

  1. override other methods from LinkedList where dataset is changing and set 'dirty = true'
  2. remove ' if (!dirty && sanArray != null)' and always recreate array from dataset
  3. make method 'setDitry(boolean)' - and user can call it before any other methods, like 'remove' etc.

Also I propose to make variables 'protected' instead of 'private' if variable don't have getters/setters. Then users of this library can get access to variables through inheritance.

Thanks, Eugene

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bhlangonijr/chesslib/issues/6, or mute the thread https://github.com/notifications/unsubscribe-auth/AA6mDYCBN_7ZwXnkz89J-ahHAfPZYFvVks5uMZukgaJpZM4VqYpn .

bhlangonijr commented 4 years ago

Fixed