Skip to content

Conversation

@davsclaus
Copy link
Contributor

Description

Target

  • I checked that the commit is targeting the correct branch (Camel 4 uses the main branch)

Tracking

  • If this is a large change, bug fix, or code improvement, I checked there is a JIRA issue filed for the change (usually before you start working on it).

Apache Camel coding standards and style

  • I checked that each commit in the pull request has a meaningful subject line and body.
  • I have run mvn clean install -DskipTests locally from root folder and I have committed all auto-generated changes.

@github-actions
Copy link
Contributor

🌟 Thank you for your contribution to the Apache Camel project! 🌟

🤖 CI automation will test this PR automatically.

🐫 Apache Camel Committers, please review the following items:

  • First-time contributors require MANUAL approval for the GitHub Actions to run

  • You can use the command /component-test (camel-)component-name1 (camel-)component-name2.. to request a test from the test bot.

  • You can label PRs using build-all, build-dependents, skip-tests and test-dependents to fine-tune the checks executed by this PR.

  • Build and test logs are available in the Summary page. Only Apache Camel committers have access to the summary.

  • ⚠️ Be careful when sharing logs. Review their contents before sharing them publicly.

Copy link
Contributor

@Croway Croway left a comment

Choose a reason for hiding this comment

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

Code Review

Summary

This PR adds a concat() function to the Simple language for concatenating expressions with an optional separator. The implementation is solid with good test coverage.

Issues Found

1. Typo in error message - missing closing brace

File: SimpleFunctionExpression.java (lines 244 and 281)

"Valid syntax: ${concat(exp)} or ${concat(exp,exp) or ${concat(exp,exp,separator)} was: "
//                                              ^ missing closing brace

Should be: ${concat(exp,exp)} (with the closing })

2. Confusing parameter naming: right and left are semantically backwards

Files: CSimpleHelper.java:153, SimpleExpressionBuilder.java:193

The parameters are named right and left, but right appears first in the output:

public static String concat(Exchange exchange, Object right, Object left, Object separator) {
    // ...
    return val1 + (sep != null ? sep : "") + val2;  // right comes first, left comes second
}

This is counterintuitive - typically left would be the first operand. Consider renaming to first/second or exp1/exp2.

3. Inconsistent use of before vs beforeLast

File: SimpleFunctionExpression.java

  • Line 241 (Simple parser): StringHelper.before(remainder, ")")
  • Line 279 (CSimple code gen): StringHelper.beforeLast(remainder, ")")

This could cause different parsing behavior with nested parentheses. Is this intentional?

Verdict

The implementation works well and has good test coverage. The error message typo should be fixed. The parameter naming is a readability concern but not a blocker.

@davsclaus davsclaus merged commit 073d168 into main Jan 12, 2026
5 checks passed
@davsclaus
Copy link
Contributor Author

Thanks for the review

@davsclaus davsclaus deleted the con branch January 12, 2026 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants