lorenzleutgeb
commented
3 years ago 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 ArrayListThese 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.
AntoniusW
madmike200590
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:
BinaryNoGoodPropagationEstimation.BasicSubstitution.Refactoring required:
Alpha#getConfigis never used and can be removed.AnswerSetQuery#forPredicate(Predicate)and the interface contains superfluouspublicmodifiers.ProgramParser#parse(Map<String, PredicateInterpretation> externalPredicateDefinitions, Path... programSources)is the sole overloadedparsemethod where external predicates is the first parameter (probably due to varargsPath...). This should be in line and varargs avoided with aPath[]array as first parameter.Refactoring suggestions:
NormalProgramfrom API and keep it only in core.NormalProgramis contained inDebugSolvingContextand written inMain.javabut anASPCore2Programwould suffice there.java.util.function.Predicate<Predicate> filterandAnswerSetPredicateFiltermay be more telling.AnswerSetcould support methodMap<Predicate,SortedSet<Atom>> getInstances()while method#getPredicateInstancescould be renamed to#getInstancesOfPredicate(Predicate).ComparisonOperatorshould 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,ComparisonOperatorsreplaced a switching of the type of operator with sub-classing, spreading the behaviour ofComparisonOperator#compareover six classes -- a simple switch in one place probably is much more understandable and avoids a lot of boilerplate. Additionally, inAggregateLiteralSplittingan if-else-if chain could be replaced with a switch on the enum (again).MinusTermofArithmeticTermImplseems superfluous.AtomandLiteralhierarchies (not all interface methods make sense on every atom or literal). This should also consider dissolvingVariableNormalizableAtomas its only method is implemented by all atom implementations. Consider whether a generic form of literalLiteral<SpecificAtomType>makes sense.BasicSubstitutionreally should be in commons and not just in core and available through a factory in commons. Some methods ofSubstitutionmay have better names.AbstractProgramshould implementProgram.Predicate#isInternalandPredicate#isSolverInternallikely should not be in API as their names already suggest.ComponentGraph#SCCComponentare not named very intuitively.DependencyGraphis actually a predicate-dependency graph, this should be reflected in its name. Furthermore,DependencyGraph#Node#getLabelis only an alternative name forNode#getPredicate#toStringand probably can be removed.Unifiercurrently extendsSubstitution, but maybe should be the other way round asSubstitutionis actually only for grounding substitutions and unifier is a substitution that allows mappings to variables, not just ground terms. AsSubstitution#evalis used at the core of all grounding, care must be taken to not impact performance negatively with those changes.Predicatereally needs methodsisInternal()andisSolverInternal().AggregateElementrepresents.Atom#substitutemethods, also document them.AnswerSetQueryallows to set filters but lacks the most important one, namely whichPredicatethis 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.DisjunctiveHeador implement a rewriting in case input program is head-cycle-free.Literals#fromAtomis a duplicate ofAtom#toLiteralfunctionality and remove if so.at.ac.tuwien.kr.alpha.commons.substitutions.Instancefrom api/commons view and move to core.TODOcomments in code.Terms#renameTermsis mis-named as it callsnormalizeVariableswhich in turn renames only variable terms.Terms#evaluateGroundTermto make clear that this is arithmetic evaluation yielding integers.Terms#new..TermvsPredicate#getPredicatevsAtoms#new..AtomvsLiterals#fromAtom. Previously most of these used a#getInstancenaming.CompiledRuleandCompiledProgramexists since both are in alpha-core where their only implementationsInternalRuleandInternalProgramalso exist. It seems that both interfaces can be deleted.BridgedGrounderandBridgearchitecture of the grounder could be removed, since it is not in use any longer and Alpha nowadays has its own mechanism for external predicates.RuleGroundingOrderandRuleGroundingInfocan be reverted as this is pretty internal information and only used in the core.core.Programsprovides one single helper method (of 2 lines of code) that is only used in one test, maybe the class should be inlined completely.[Interface]ImplandBasic[Interface]but we also have interfaces namedBasicAtomwhich gives rise toBasicAtomImpl.AtomCountershould count atom types based on theirStringrepresentation or onClassobjects.parseAndPreprocessfunction, it looks like this could be shared among those tests.ASPCore2Programwhose sole implementation isInputProgram, which is not obvious from the naming.Improve/Complete documentation in: