Copilot commented on code in PR #6473:
URL:
https://github.com/apache/incubator-kie-drools/pull/6473#discussion_r2400994845
##########
drools-model/drools-model-codegen/src/main/java/org/drools/model/codegen/execmodel/generator/Consequence.java:
##########
@@ -265,10 +267,17 @@ private Set<String> extractUsedDeclarations(BlockStmt
ruleConsequence, String co
}
if (context.getRuleDialect() == RuleContext.RuleDialect.MVEL) {
- return existingDecls.stream().filter(d -> containsWord(d,
consequenceString)).collect(toSet());
+ List<String> ordered = existingDecls.stream().filter(d ->
containsWord(d, consequenceString)).sorted((a, b) ->
Integer.compare(consequenceString.indexOf(a),
consequenceString.indexOf(b))).collect(Collectors.toList());
Review Comment:
The sorting logic calls `consequenceString.indexOf()` multiple times for the
same strings during comparison. Consider pre-computing the indices in a Map to
avoid repeated string searches, especially for longer consequence strings with
many declarations.
```suggestion
// Precompute indices for each declaration in the consequence
string
Map<String, Integer> declIndices = new HashMap<>();
for (String decl : existingDecls) {
if (containsWord(decl, consequenceString)) {
declIndices.put(decl, consequenceString.indexOf(decl));
}
}
List<String> ordered = declIndices.keySet().stream()
.sorted((a, b) -> Integer.compare(declIndices.get(a),
declIndices.get(b)))
.collect(Collectors.toList());
```
##########
drools-model/drools-model-codegen/src/main/java/org/drools/model/codegen/execmodel/generator/Consequence.java:
##########
@@ -265,10 +267,17 @@ private Set<String> extractUsedDeclarations(BlockStmt
ruleConsequence, String co
}
if (context.getRuleDialect() == RuleContext.RuleDialect.MVEL) {
- return existingDecls.stream().filter(d -> containsWord(d,
consequenceString)).collect(toSet());
+ List<String> ordered = existingDecls.stream().filter(d ->
containsWord(d, consequenceString)).sorted((a, b) ->
Integer.compare(consequenceString.indexOf(a),
consequenceString.indexOf(b))).collect(Collectors.toList());
+ return new LinkedHashSet<>(ordered);
} else if (ruleConsequence != null) {
- Set<String> declUsedInRHS =
ruleConsequence.findAll(NameExpr.class).stream().map(NameExpr::getNameAsString).collect(toSet());
- return
existingDecls.stream().filter(declUsedInRHS::contains).collect(toSet());
+ List<String> rhsNamesInOrder =
ruleConsequence.findAll(NameExpr.class).stream().map(NameExpr::getNameAsString).collect(Collectors.toList());
+ LinkedHashSet<String> ordered = new LinkedHashSet<>();
+ for (String n : rhsNamesInOrder) {
+ if (existingDecls.contains(n)) {
+ ordered.add(n);
+ }
+ }
Review Comment:
The loop performs `existingDecls.contains(n)` lookup for each name. If
`existingDecls` is a large collection, consider converting it to a HashSet
first for O(1) lookup performance, or use a stream with filter operation for
better readability.
```suggestion
LinkedHashSet<String> ordered = rhsNamesInOrder.stream()
.filter(existingDecls::contains)
.collect(Collectors.toCollection(LinkedHashSet::new));
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
