Skip to content

Conversation

@mattcoley
Copy link
Collaborator

Take 2. I will fix this up.

interpreter.getContext().removeResolvedExpression(expression);

return result;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an unused method parameter in this method. Should we remove it? Map<String, Object> kwargs

}

private String generateTempVariable(String tempValue, String expression) {
return String.format("%s.%s", tempValue, expression).trim();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The String.format has some performance issues. So, for curiosity, I ran a benchmark to compare the approaches.

Those are the results:

Benchmark                                                Mode  Cnt   Score   Error  Units
StringConcatSimulation.concatWithConcatFunction            ss   25   1.900 ± 0.120  ms/op
StringConcatSimulation.concatWithPlus                      ss   25   2.079 ± 0.438  ms/op
StringConcatSimulation.concatWithStringBuilder             ss   25   1.871 ± 0.436  ms/op
StringConcatSimulation.concatWithStringBuilderWithCount    ss   25   1.720 ± 0.255  ms/op
StringConcatSimulation.concatWithStringFormat              ss   25  15.145 ± 2.836  ms/op

I would suggest using StringBuilder instead.

Suggested change
return String.format("%s.%s", tempValue, expression).trim();
return new StringBuilder()
.append(tempValue)
.append(".")
.append(expression)
.toString()
.trim();

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 this pull request may close these issues.

3 participants