bhlangonijr / chesslib

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

PgnLoaderTest fix #25

Closed edugarciagt closed 3 years ago

edugarciagt commented 4 years ago

Found there where some problems when runing PgnLoaderTest. I manage to fix it by changing a little the way function parse the line as follows.

Keep checking other errors. I hope this helps.

Interesting I found you put games by round, I don't see the reason. Better aproach would be using a simple List and put games there, then in order to find similar games using Lambda stream functions to filter the list by round, event, player, etc. Let me know if you agree with that.

public static PgnProperty parsePgnProperty(String line) {
        try {
            String parsedLine = line.trim().replace("[", "");
            parsedLine = parsedLine.replace("]", "");
            parsedLine = parsedLine.replace("\"", "");
            String[] values = parsedLine.split(" ",2);

            return new PgnProperty(values[0], values[1]);
        } catch (Exception e) {
            // do nothing
        }

        return null;
    }
bhlangonijr commented 4 years ago

Hi @edugarciagt, what are the problems you get when running the unit tests? Would you be able to provide a PGN file in which I can reproduce the issues you see and potentially add your fix/test for the problem? You may also provide a PR with those and I can merge it.

Regarding your suggestion, PgnHolder will probably be deprecated in the future in favor of PgnIterator. Example:

    PgnIterator games = new PgnIterator("/path/file.pgn");
    for (Game game : games) {
      // do work here
    }  

Mind that one might need to load very large PGN files. Ideally, you should iterate over a defined number of games and add it to a List yourself for further manipulation, similar to how we manage paginated results of a database. Loading the entire PGN file into a list would be prohibitive for such large files. It'd quickly overwhelm your computer resources for a moderately large file. PgnIterator can handle very large files. You must process the games in chunks though, cleaning up or leaving the intermediate objects loaded eligible for garbage collection. That said, it's completely reasonable your suggestion to have a facility that loads the games into a List so that you can conveniently filter and manipulate it using lambdas or any other framework. Although for that to work it'd require designing an API to apply those filters and part of the processing while reading the PGN files and before loading them into the java objects. Some smart decisions would have to be taken to process the loaded results in a chunked fashion in order to not retain everything in memory as it'd defeat the purpose of such API.

edugarciagt commented 4 years ago

Hi, you are right, performance is better with Iterators. Let me put in order those changes I made to create a PR this week. By the way, I also added capacity to read clock time, move and position valuations by engine, and would like to put them in different PR so not having a mess with changes.

As I put changes, will make some suggestions regarding use of static methods when performing parsing tasks.

Best Regards!