Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Review feedback #2

Closed
J3173 opened this issue Jul 23, 2020 · 1 comment · Fixed by #3
Closed

Review feedback #2

J3173 opened this issue Jul 23, 2020 · 1 comment · Fixed by #3

Comments

@J3173
Copy link
Contributor

J3173 commented Jul 23, 2020

My first round of feedback:

This is unused.

public AQLValidationException(Exception e) {
super(e);
}

This is unused.

@Override
public void validate() {
new ArchetypeHRID(getValue());
}

This throws an IllegalArgumentException instead of an AQLValidationException as other validate methods do. Please catch the IllegalArgumentException and convert it to an AQLValidationException.

public ClassExprOperand getClassExprOperand() {
return getLookup().getClassExprOperand(getVariableName());
}

This is unused. I'm not sure why.

public Boolean isCompositionPath() {
String identifier = pathParts.get(0).getIdentifier().toLowerCase();
//Check if first identifier equals
switch (identifier) {
case "language":
case "territory":
case "category":
case "context":
case "composer":
case "content":
return true;
default:
return false;
}
}

Some of these attributes are also attributes on other RM classes, e.g. language is also an attribute on OBSERVATION and DV_TEXT. Can't this be solved another way?

import java.time.LocalDateTime;
import java.time.OffsetDateTime;

These are unused.

public Operator(String type) {
this.type = OperatorType.get(type);
}

This is unused.

public String getParameterKey(PrimitiveOperand primitiveOperand) {
return parameter.entrySet().stream().filter(primitiveOperand::equals).map(Map.Entry::getKey).findFirst().get();
}

I don't think this works. I think you mean

filter(entry -> primitiveOperand.equals(entry.getValue()))

protected void addChild(ParseTree parseTree) {

This is unused.

All protected methods in NodeExpression can be changed to private.

final QOMParser visitor = new QOMParser(lookup);
QOMObject object = visitor.visit(parseTree);

can be changed to

QOMObject object = QOMParserUtil.parse(lookup, parseTree);

Then this class doesn't need to extend AQLBaseVisitor anymore and private Lookup lookup;, the constructor and public QOMObject visitChildren(RuleNode node) { ... } can be removed.

@boeckers
Copy link
Contributor

I processed your comments in a separate review branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants