Hello. Sorry for the delay in reviewing.
Le lun. 18 oct. 2021 à 09:35, Avijit Basak <avijit.ba...@gmail.com> a écrit : > > Hi All > > I have created PR#197 as mentioned earlier. Kindly let me know if > there is any concern or comments. > I have created another *PR#199* consisting of the changes with > adaptive probability generations. Please find below my first remarks. (A) Please "rebase" on "master". Please "squash" intermediate commits: For a new feature, a single commit should exist (that corresponds to the JIRA report describing it). (B) I'm confused by your defining "legacy" packages in new modules... (C) File "commons-math-examples/examples-ga/src/main/resources/spotbugs/spotbugs-exclude-filter.xml" does not belong there. (D) General design Class "ConsoleLogger" -> We should not reinvent the wheel. We should consider whether logging is necessary, and in the affirmative, depend on the de facto standard: "slf4j". Class "Constants" -> Any data should be declared where its purpose is obvious. * Class "RandomGenerator" 1. Duplicates functionality (storage of thread-local instances) readily available in "Commons RNG". 2. (IMHO) Thread-local instances should not be used for "heavy" usage (like in GA). * Class "ValidationUtils" -> should not be public (or should be defined in an "internal" package). * Class "AbstractListChromosome" (and subclasses) Didn't we conclude that this was a very wasteful implementation of the "chromosome" concept? * Package "convergencecond" -> Should probably be renamed "convergence". * Class "GeneticException" 1. Should not be public (or should be defined in an "internal" package"[1]). 2. If various types (that map to different JDK subclasses of RuntimeException, e.g. "IllegalArgumentException" vs "NullPointerException") are necessary, there could be a factory for creating the appropriate instance. However, for "null" checks, please use the JDK utilities[2]. * Class "ConvergenceListenerRegistry" Shouldn't it be thread-safe? (E) Unit tests * src/test 1. New tests should use Junit 5. 2. "Example usages" probably belong in the "examples-ga" module. (F) Code readability * Please write one argument per line. * Write one condition check per line. * Avoid comments with no added value (like "constructor" for a constructor). * Avoid "ASCII art" (see e.g. "OnePointCrossover"); a link[2] is often preferable. * Do no duplicate documentation (see e.g. "OnePointCrossover"). (G) Some files contain "tab" characters (e.g. "pom.xml"). Best regards, Gilles [1] https://gitbox.apache.org/repos/asf?p=commons-math.git;a=tree;f=commons-math-neuralnet/src/main/java/org/apache/commons/math4/neuralnet/internal;h=c0bc1409cdee0932c973fae05f03e8288034c005;hb=HEAD [2] https://docs.oracle.com/javase/8/docs/api/java/util/Objects.html#requireNonNull-T- [3] https://en.wikipedia.org/wiki/Crossover_(genetic_algorithm) --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
