Support of nested expressions?

[benze]

Support of nested expressions?

[squeek502]

Why not evaluate them separately?

int var1 = 10;

Expression innerExpression = new Expression("5 * var1").with("var1", new BigDecimal(var1));
BigDecimal innerResult = innerExpression.eval();

Expression outerExpression = new Expression("25 + var2").with("var2", innerResult);
BigDecimal outerResult = outerExpression.eval();

System.out.println(outerResult.toPlainString());

[uklimaschewski]

Adding a function

public Expression with(String variable, Expression value) {
    return setVariable(variable, value.eval());
}

might be the quickest way.

It would allow a call like

var1 = 10;
var2 = "5 * var1";
var3 = "25 + var2";
Expression e = new Expression( var3 ).with( "var2", new Expression(var2).with( "var1", var1));
e.eval();

A more sophisticated way might be to check in the with(String, String) method, if the string is an expression and if, then evaluate it first.

[benze]

The whole catch is that I don't want to have to know what the expression is myself ahead of time, and I don't want to parse it manually.

For instance, if a user is entering the functions as raw data through an online form, his formula may use a var which itself is a formula.

I tried quickly to update the with(String, String) method and if the string wasn't a BigDecimal, then to add it to the variables list as an Expression. Then in the eval() method, when looping over the variables, if it found an expression, to eval the expression, but the RPN generating method threw an exception b/c it is obviously not expecting an Expression as a variable. And I wasn't able to follow the Shunting Yard algorithm well enough to modify that as well.

Then I realized that I might be complicating things a little too much, or perhaps there was more thought that was required, in case the nested expression needed access to the parent's vars (should that even be allowed?), etc.

[uklimaschewski]

Benze, please have a look at my latest commit. I think I found an easier way to support nested expressions. I have included your unit test and it evaluates correctly.
I simply do a string replacement (with braces to isolate the nested expression) and invalidate any previous RPN creation.

[benze]

My first instinct when I saw your solution was wow - simple! Great! But I had a nagging feeling that there might be some conditions in which it would fail - if var is declared before the sub-expression that it is dependent on, since you are doing replacement when the var is added, and not at the end during the execution phase.

Ex:

   @Test
    public void testNestedVars2(){
        String exp = "3 * z";
        String b = "c";
        String z = "a + b";
        String a = "10";
        String c = "5";

        Expression e = new Expression(exp);
        e.with("b", b);
        e.with("z", z);
        e.with("a", a);
        e.with("c", c);

        assertEquals("45", e.eval().toString());
    }

Since my proposed solution does all the replacement only at execution phase, everything is already defined. Although I much preferred the simplicity in your solution.

I added a recursion limit check to my solution as well and an extra circular argument test to the TestNested class to limit recursion depths and stack overflows

[uklimaschewski]

I don't see a reason for having to support an e.with("x","y") on an expression which does not have an "x" already in it.

[benze]

Strange. I was sure I had already responded to this, but the response seems to have disappeared.

The problem with your solution is that it requires the variables to be entered in a specific order. In the test example above, the order makes all the difference. Inverting b & z definitions makes the test successful.

With my proposed commit, the order becomes irrelevant, since the evaluation is done at execution only. Your solution requires the user to pre-order the variables before setting them in the expression, which can be problematic for the user and the application.

[uklimaschewski]

I thought quite a while about it, but I think this feature is so rarely needed, and possibly easier managed on a higher level, so that it does not justify such a change here.
I would like to keep this little class handy and easy to understand and maintain.
While I apreciate any input and improvement, I am sorry, but I have to reject this pull request.

[benze]

I respectfully disagree, but that's ok. I will use my forked version for my own needs.

Thanks,

Eric