Alpha Modularized code quality needs improvement.

Here we track code quality issues that became visible due to the modularization. The goal is to fix those issues after the basic modularization has been merged into master, as addressing all of them before merging would significantly increase the scope of changes already in PR #274. The lists here will be expanded during code review and modified according to ongoing discussions.

Documentation missing:

[x] alpha-api/src/main/java/at/ac/tuwien/kr/alpha/api/Alpha.java lacks all documentation.
[x] alpha-api/src/main/java/at/ac/tuwien/kr/alpha/api/AnswerSet.java lacks all documentation.
[x] alpha-api/src/main/java/at/ac/tuwien/kr/alpha/api/AnswerSetQuery.java needs better documentation.
[x] alpha-api/src/main/java/at/ac/tuwien/kr/alpha/api/ComparisonOperator.java lacks all documentation.
[x] alpha-api/src/main/java/at/ac/tuwien/kr/alpha/api/DebugSolvingContext.java lacks all documentation.
[x] alpha-api/src/main/java/at/ac/tuwien/kr/alpha/api/StatisticsReportingSolver.java needs full documentation.
[x] alpha-api/src/main/java/at/ac/tuwien/kr/alpha/api/config/AggregateRewritingConfig.java needs documentation.
[x] alpha-api/src/main/java/at/ac/tuwien/kr/alpha/api/config/BinaryNoGoodPropagationEstimationStrategy.java should be more detailed with information from BinaryNoGoodPropagationEstimation.
[x] alpha-api/src/main/java/at/ac/tuwien/kr/alpha/api/programs/InlineDirectives.java needs documentation.
[x] alpha-api/src/main/java/at/ac/tuwien/kr/alpha/api/grounder/Substitution.java needs documentation that is partially in BasicSubstitution.
[x] alpha-api/src/main/java/at/ac/tuwien/kr/alpha/api/programs/ASPCore2Program.java needs documentation.
[x] alpha-api/src/main/java/at/ac/tuwien/kr/alpha/api/programs/NormalProgram.java needs documentation.
[x] alpha-api/src/main/java/at/ac/tuwien/kr/alpha/api/programs/Program.java needs documentation.
[x] alpha-api/src/main/java/at/ac/tuwien/kr/alpha/api/programs/Predicate.java needs documentation.
[x] alpha-api/src/main/java/at/ac/tuwien/kr/alpha/api/programs/ProgramParser.java needs documentation.
[x] alpha-api/src/main/java/at/ac/tuwien/kr/alpha/api/programs/analysis/ComponentGraph.java needs documentation.
[x] alpha-api/src/main/java/at/ac/tuwien/kr/alpha/api/programs/analysis/DependencyGraph.java needs documentation.
[ ] alpha-api/src/main/java/at/ac/tuwien/kr/alpha/api/programs/literals/AggregateLiteral.java needs documentation.
[ ] alpha-api/src/main/java/at/ac/tuwien/kr/alpha/api/programs/literals/BasicLiteral.java
[ ] alpha-api/src/main/java/at/ac/tuwien/kr/alpha/api/programs/literals/ComparisonLiteral.java
[ ] alpha-api/src/main/java/at/ac/tuwien/kr/alpha/api/programs/literals/ExternalLiteral.java

Refactoring required:

[x] Alpha#getConfig is never used and can be removed.
[x] Interface lacks method AnswerSetQuery#forPredicate(Predicate) and the interface contains superfluous public modifiers.
[x] ProgramParser#parse(Map<String, PredicateInterpretation> externalPredicateDefinitions, Path... programSources) is the sole overloaded parse method where external predicates is the first parameter (probably due to varargs Path...). This should be in line and varargs avoided with a Path[] array as first parameter.

Refactoring suggestions:

[ ] Remove NormalProgram from API and keep it only in core. NormalProgram is contained in DebugSolvingContext and written in Main.java but an ASPCore2Program would suffice there.
[ ] Introduce a class/interface for the filtering of predicates from answer sets, currently it occurs as java.util.function.Predicate<Predicate> filter and AnswerSetPredicateFilter may be more telling.
[ ] Interface AnswerSet could support method Map<Predicate,SortedSet<Atom>> getInstances() while method #getPredicateInstances could be renamed to #getInstancesOfPredicate(Predicate).
[ ] Interface ComparisonOperator should probably be replaced with an enum in API and just state the standard operations. There should to be no need for arbitrary comparison operators. Additionally, ComparisonOperators replaced a switching of the type of operator with sub-classing, spreading the behaviour of ComparisonOperator#compare over six classes -- a simple switch in one place probably is much more understandable and avoids a lot of boilerplate. Additionally, in AggregateLiteralSplitting an if-else-if chain could be replaced with a switch on the enum (again).
[ ] Inner class MinusTerm of ArithmeticTermImpl seems superfluous.
[ ] Revisit and potentially refactor Atom and Literal hierarchies (not all interface methods make sense on every atom or literal). This should also consider dissolving VariableNormalizableAtom as its only method is implemented by all atom implementations. Consider whether a generic form of literal Literal<SpecificAtomType> makes sense.
[ ] Check whether BasicSubstitution really should be in commons and not just in core and available through a factory in commons. Some methods of Substitution may have better names.
[ ] Check if AbstractProgram should implement Program.
[ ] Predicate#isInternal and Predicate#isSolverInternal likely should not be in API as their names already suggest.
[ ] The methods in ComponentGraph#SCCComponent are not named very intuitively.
[ ] DependencyGraph is actually a predicate-dependency graph, this should be reflected in its name. Furthermore, DependencyGraph#Node#getLabel is only an alternative name for Node#getPredicate#toString and probably can be removed.
[ ] Unifier currently extends Substitution, but maybe should be the other way round as Substitution is actually only for grounding substitutions and unifier is a substitution that allows mappings to variables, not just ground terms. As Substitution#eval is used at the core of all grounding, care must be taken to not impact performance negatively with those changes.
[ ] Check if Predicate really needs methods isInternal() and isSolverInternal().
[ ] alpha-api/src/main/java/at/ac/tuwien/kr/alpha/api/programs/atoms/AggregateAtom.java should be better documented and with examples, e.g., what an AggregateElement represents.
[ ] Consider reworking the Atom#substitute methods, also document them.
[ ] The interface for AnswerSetQuery allows to set filters but lacks the most important one, namely which Predicate this query is applied to. This is currently based in the factory method to construct a query, but would make more sense in place where all other conditions for the query are set.
[ ] Remove DisjunctiveHead or implement a rewriting in case input program is head-cycle-free.
[ ] Check whether Literals#fromAtom is a duplicate of Atom#toLiteral functionality and remove if so.
[ ] Consider hiding at.ac.tuwien.kr.alpha.commons.substitutions.Instance from api/commons view and move to core.
[ ] Get rid of TODO comments in code.
[ ] Check if Terms#renameTerms is mis-named as it calls normalizeVariables which in turn renames only variable terms.
[ ] Rename Terms#evaluateGroundTerm to make clear that this is arithmetic evaluation yielding integers.
[ ] In commons module there are creation factories for terms, predicates, atoms, and literals using different naming schemas. This probably should be unified. Terms#new..Term vs Predicate#getPredicate vs Atoms#new..Atom vs Literals#fromAtom. Previously most of these used a #getInstance naming.
[ ] It is not clear why interfaces for CompiledRule and CompiledProgram exists since both are in alpha-core where their only implementations InternalRule and InternalProgram also exist. It seems that both interfaces can be deleted.
[ ] The whole BridgedGrounder and Bridge architecture of the grounder could be removed, since it is not in use any longer and Alpha nowadays has its own mechanism for external predicates.
[ ] The extraction of interfaces RuleGroundingOrder and RuleGroundingInfo can be reverted as this is pretty internal information and only used in the core.
[ ] core.Programs provides one single helper method (of 2 lines of code) that is only used in one test, maybe the class should be inlined completely.
[ ] A unified naming schema for interfaces and their default implementations should be kept throughout all components of Alpha. Currently we have [Interface]Impl and Basic[Interface] but we also have interfaces named BasicAtom which gives rise to BasicAtomImpl.
[ ] Check whether AtomCounter should count atom types based on their String representation or on Classobjects.
[ ] Several tests need a parser, a normalizer and define a parseAndPreprocess function, it looks like this could be shared among those tests.
[ ] Consider naming conventions with respect to ASPCore2Program whose sole implementation is InputProgram, which is not obvious from the naming.

Improve/Complete documentation in:

[ ] alpha-api/src/main/java/at/ac/tuwien/kr/alpha/api/programs/literals/Literal.java
[ ] alpha-api/src/main/java/at/ac/tuwien/kr/alpha/api/rules/Rule.java
[ ] alpha-api/src/main/java/at/ac/tuwien/kr/alpha/api/rules/heads/ChoiceHead.java
[ ] alpha-api/src/main/java/at/ac/tuwien/kr/alpha/api/terms/FunctionTerm.java
[ ] alpha-api/src/main/java/at/ac/tuwien/kr/alpha/api/terms/ArithmeticTerm.java
[ ] alpha-api/src/main/java/at/ac/tuwien/kr/alpha/api/terms/Term.java
[ ] alpha-commons/src/main/java/at/ac/tuwien/kr/alpha/commons/atoms/Atoms.java
[ ] alpha-commons/src/main/java/at/ac/tuwien/kr/alpha/commons/literals/BasicLiteralImpl.java
[ ] alpha-commons/src/main/java/at/ac/tuwien/kr/alpha/commons/literals/AbstractLiteral.java
[ ] alpha-commons/src/main/java/at/ac/tuwien/kr/alpha/commons/literals/Literals.java
[ ] alpha-commons/src/main/java/at/ac/tuwien/kr/alpha/commons/rules/heads/Heads.java
[ ] alpha-commons/src/main/java/at/ac/tuwien/kr/alpha/commons/rules/heads/NormalHeadImpl.java

Oct 15 '21 01:10 AntoniusW

Because the following involves benchmarking, it might warrant a separat issue, but nevertheless, I uncovered it during the review for #274, so here we go:

C:\Users\lorenz\src\github.com\alpha-asp\Alpha\alpha-core\src\main\java\at\ac\tuwien\kr\alpha\core\solver\NoGoodStoreAlphaRoaming.java:78: warning: [rawtypes] found raw type: ArrayList
	private ArrayList<WatchedNoGood>[] watches = new ArrayList[0];
	                                                 ^
  missing type arguments for generic class ArrayList<E>
  where E is a type-variable:
    E extends Object declared in class ArrayList
C:\Users\lorenz\src\github.com\alpha-asp\Alpha\alpha-core\src\main\java\at\ac\tuwien\kr\alpha\core\solver\NoGoodStoreAlphaRoaming.java:80: warning: [rawtypes] found raw type: ArrayList
	private ArrayList<WatchedNoGood>[] watchesAlpha = new ArrayList[0];
	                                                      ^
  missing type arguments for generic class ArrayList<E>
  where E is a type-variable:
    E extends Object declared in class ArrayList
C:\Users\lorenz\src\github.com\alpha-asp\Alpha\alpha-core\src\main\java\at\ac\tuwien\kr\alpha\core\solver\NoGoodStoreAlphaRoaming.java:105: warning: [rawtypes] found raw type: ArrayList
		watches = new ArrayList[0];
		              ^
  missing type arguments for generic class ArrayList<E>
  where E is a type-variable:
    E extends Object declared in class ArrayList
C:\Users\lorenz\src\github.com\alpha-asp\Alpha\alpha-core\src\main\java\at\ac\tuwien\kr\alpha\core\solver\NoGoodStoreAlphaRoaming.java:106: warning: [rawtypes] found raw type: ArrayList
		watchesAlpha = new ArrayList[0];
		                   ^
  missing type arguments for generic class ArrayList<E>
  where E is a type-variable:
    E extends Object declared in class ArrayList

These warnings can be avoided by using ArrayList<ArrayList<WatchedNoGood>> instead of ArrayList<WatchedNoGood>[]. It's not obvious how the two variants differ in performance, so I'd ask for benchmarking, but at the same time I conjecture that the impact would be neglegible.

Oct 21 '21 10:10 lorenzleutgeb

I created some issues that were uncovered in #274: #302, #303, #304.

Oct 21 '21 11:10 lorenzleutgeb

Because the following involves benchmarking, it might warrant a separat issue, but nevertheless, I uncovered it during the review for #274, so here we go:
C:\Users\lorenz\src\github.com\alpha-asp\Alpha\alpha-core\src\main\java\at\ac\tuwien\kr\alpha\core\solver\NoGoodStoreAlphaRoaming.java:78: warning: [rawtypes] found raw type: ArrayList
	private ArrayList<WatchedNoGood>[] watches = new ArrayList[0];
	                                                 ^
  missing type arguments for generic class ArrayList<E>
  where E is a type-variable:
    E extends Object declared in class ArrayList
...
These warnings can be avoided by using ArrayList<ArrayList<WatchedNoGood>> instead of ArrayList<WatchedNoGood>[]. It's not obvious how the two variants differ in performance, so I'd ask for benchmarking, but at the same time I conjecture that the impact would be neglegible.

Agreed, this should not be changed without benchmarking. Since access to watches is at the core of propagation, this is (part of) the code that runs millions of times per second. Arrays have a small advantage over ArrayLists, namely that the array (in theory) needs only one memory access to get the desired data, while the ArrayList is an object that stores a reference to an array, hence there are two memory accesses required to get the data. Since the outer list is accessed randomly (while the inner one will be walked sequentially), I conjecture that changing the outer to an ArrayList might slow down propagation. But at the moment, I also do not have solid data for that behaviour with the current solver. For the moment a separate issue might be advised.

Oct 21 '21 12:10 AntoniusW

One more: Allow configuration of packages to be scanned for external predicates.

Oct 26 '21 13:10 lorenzleutgeb

alpha-api/src/main/java/at/ac/tuwien/kr/alpha/api/StatisticsReportingSolver.java needs full documentation.

@AntoniusW could you please take care of this? I lack the level of detailed expertise in our DefaultSolver to properly document the collected statistics.

Nov 08 '21 12:11 madmike200590

alpha-api/src/main/java/at/ac/tuwien/kr/alpha/api/config/BinaryNoGoodPropagationEstimationStrategy.java should be more detailed with information from BinaryNoGoodPropagationEstimation.

The current javadoc for both of these is not approachable to users without in-depth knowledge of ASP solver architecture. Any API user wanting to use anything other than default settings here will have to read some research papers as well as sources of the alpha-core module, so I'm at a loss on what to put into the javadoc here (especially since I'm not completely sure this should even be public API).

Nov 09 '21 12:11 madmike200590

Interface lacks method AnswerSetQuery#forPredicate(Predicate) and the interface contains superfluous public modifiers.

AnswerSetQuery#forPredicate(Predicate) is a static factory method that does not make sense in an interface definition.

Nov 09 '21 12:11 madmike200590

Moving a TODO here:

class BasicLiteralImpl extends AbstractLiteral implements BasicLiteral
// TODO could we parameterize Literal with Atom type?

Dec 01 '21 14:12 lorenzleutgeb