A few thoughts on JSR331 while implementing it for my solver
Julien Vion
I am quite fine with what has been done with the Problem, Var and Constraint interfaces, except from minor issues that will probably rather impact users than developers. I had much more trouble with the Solver/SearchStrategy interfaces due to Concrete's architecture. I will start with these, as they imply big changes to the specification that are probably not desirable. I only give them just in case it raises new ideas.
− Concrete uses separate Variable and Constraint classes for modeling and solving, as I want my modeling API to support reformulation (adding and removing variable and constraints easily), and I want my solving API to use immutable structures as much as possible for speed and safety. This scheme is used by other solvers such as Choco. However, there is no bijection between the "solving" and "modeling" classes, as transformations can be applied upon loading depending on the solver's version. As SearchStrategy is designed to expect the same classes for modeling and solving, it makes custom search strategies for Concrete almost impossible to implement properly with JSR331.
− Changes in the domain during search are not passed to the "modeling" classes (the solver just returns solutions), which make some domain-aware methods such as isBound() not working as expected. store and restoreProblem's behavior seems to make many assumptions on how solvers handle backtracking.
− Concrete is mostly parameterized globally (it makes it far easier to add new parameters and adjust them from the command line during benchmarking, and Concrete has never been designed to allow custom search strategies easily BTW). It makes the SearchStrategies awkward to apply to one instance of the solver. Well, I think that Concrete might actually been improved there.
Now a few thoughts about the Problem, Var and Constraint interfaces:
About the methods in Problem:
− public Constraint post(String name, String symbolicExpression)
This
method should be documented to throw an exception if the
expression cannot be parsed correctly (and never returning null, which would probably
entails a confusing NullPointerException anyway).
− public Var createVariable(String name, int min, int max)
I do not understand what the difference is with the variable() method; no difference appear in the JavaDoc. If it implies that a Var instance is created but not added to the problem, it should be documented, and I cannot imagine a use case for this BTW.
− public Constraint post(int[] array, Var[] vars, String oper, int value)
− public Constraint post(int[] array, VarList vars, String oper, int value)
− public Constraint post(int[] array, Var[] vars, String oper, Var var)
− public Constraint post(Var[] vars, String oper, int value)
− public Constraint post(VarList vars, String oper, int value)
− public Constraint post(Var[] vars, String oper, Var var)
− public Constraint post(VarList vars, String oper, Var var)
− public Constraint post(Var var1, Var var2, String oper, int value)
− public Constraint post(Var var1, Var var2, String oper, Var var)
These are confusing synonyms for postLinear constraints. I think it is important to state explicitly that a sum/linear constraint is involved in the method's name. Short and inexplicit method names may earn a few seconds to an expert developer that use a poor IDE, and later causes much headscratching to maintainers and debuggers. Java5+'s varargs notation could also be used to reduce the number of methods, for example:
public Constraint postLinear(Var var, String oper, Var... vars)
− public Constraint post(Var var1, String oper, Var var2)
− public Constraint postLinear(Var var1, String oper, Var var2)
This shorter "post" method is much less confusing than other linear constraints, but the synonym can be avoided for better consistency between developers and models.
The VarList class feels totally useless. It has no added value over List<Variable> (except incompatibility with the Java Collection framework).
The compareTo() method in Var is confusing. Vars are not Comparable in the way Java usually compare objects (compareTo() in the Comparable interface must define a total ordering). At the very least, comparing Vars should return a Var and not an int (as v1 > v2 can either hold or not depending on the state of the domains).
In Solver :
− getNumberOfSolutions should be getNumberOfFoundSolutions or something like that. It can make the unexpecting user think about #CSP when reading the method's name.
− solutionIterator() is much saner than findAllSolutions(), which should be avoided IMHO. The SolutionIterator class should be replaced by (or at least specialize) java.util.Iterator<Solution> for compatibility with the Java Collection API. findAllSolutions() is very likely to explode more or less unexpectedly and its use should be discouraged. If really needed, converting iterators to collections or arrays is easy and many well-known libraries such as Google Guava or Apache Commons have one-liners for this. Iterators can be processed lazily and be used to control the combinatorial explosion.
− it seems that loadFromXML/storeToXML actually do not expect the document to be XML at all… Other popular formats such as JSON or FlatZinc could be used. Methods should be renamed to loadFromInputStream/storeToOutputStream.
Finally, with the TCK: it seems that JUnit is not always used properly:
− in many cases, only one solution is assumed although other valid (e.g., symmetric) solutions can be returned by the solver.
− some Exceptions are catched, which hides the actual failed assertion or other errors from the solvers.
− assertTrue(actual == expected) is used is some places where assertEquals would be more appropriate (assertEquals generates more insightful error messages)
− when assertEquals is used, the parameters should be given in the order (expected, actual) for the correct error message.
− transition to JUnit4/Hamcrest would be nice and facilitate the work of implementers
Hope this helps,
--
Julien Vion
LAMIH CNRS, Université de Valenciennes et du Hainaut Cambrésis
