-
Notifications
You must be signed in to change notification settings - Fork 15
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
Add logical operators and expressions #57
Add logical operators and expressions #57
Conversation
This is a useful addition, as it opens the possibility to implement conditional logic, thanks. Would you mind:
Then I will consider your addition to be merged. Many thanks for your contribution. |
Hi, @bylexus, Thanks for reviewing my PR so swiftly. I followed both of your recommendations. While adding Unit tests, I spotted a few missing declarations for the new Logical Expression, which I added now, and rebuilt all the distribution files. Let me know if the tests are enough and if there is anything else I can do. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello,
Many thanks for your contribution. I have reviewed your input, and think the logical operator precedence should be changed to fit between PowerExpression (strongest), then logical expressions, then the rest. In the actual solution, the logical expressions are the weakest, which is wrong in my opinion.
Let me know what you think.
it('parses a logical expression correctly', () => { | ||
const formulaStr = '3>1'; | ||
const f = new Fparser(); | ||
let ret = f.parse(formulaStr); | ||
expect(ret).toEqual( | ||
new Fparser.LogicalExpression('>', new Fparser.ValueExpression(3), new Fparser.ValueExpression(1)) | ||
); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is correct, but it does not test the operator precedence. I think the "correct" or logical precedence of operators is:
- (strongest): Power of (x^y)
- logical operators (x<y)
- multiplication/division
- additon/ substraction
So the following formula:
3+2>1*4<2^3
should be parsed as
3+((2>1)*(4<(2^3)))
which it actually does not. So from my point of view, the following test should be green
it('evaluates the order of logical expressions correct', () => {
// 3+2>1*4<2^3 should be evaulated as:
// 3+((2>1)*(4<(2^3)))
// which evaulates to 4
const formulaStr = '3+2>1*4<2^3';
const f = new Fparser();
let ret = f.parse(formulaStr);
expect(ret).toEqual(
new Fparser.PlusMinusExpression(
'+',
new Fparser.ValueExpression(3),
new Fparser.MultDivExpression(
'*',
new Fparser.LogicalExpression(
'>',
new Fparser.ValueExpression(2),
new Fparser.ValueExpression(1)
),
new Fparser.LogicalExpression(
'<',
new Fparser.ValueExpression(4),
new Fparser.PowerExpression(new Fparser.ValueExpression(2), new Fparser.ValueExpression(3))
)
)
)
);
expect(ret.evaluate()).toBe(4);
});
Please change the precedence order in fparser.ts
, buildExpressionTree
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the other hand, if we look at other programming languages, e.g. JavaScript, the logical operators have the lowest precedence:
What do you think, what would be an appropriate handling / precedence? I would recommend to implement the general consensus, as in other languages...
// Replace all Logical expressions with a partial tree: | ||
idx = 0; | ||
expr = null; | ||
while (idx < exprCopy.length) { | ||
expr = exprCopy[idx]; | ||
if (expr instanceof LogicalExpression) { | ||
if (idx === 0 || idx === exprCopy.length - 1) { | ||
throw new Error('Wrong operator position!'); | ||
} | ||
expr.left = exprCopy[idx - 1]; | ||
expr.right = exprCopy[idx + 1]; | ||
exprCopy[idx - 1] = expr; | ||
exprCopy.splice(idx, 2); | ||
} else { | ||
idx++; | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment above: Please change the order of logical operator precedence to fit between PowerExpression and MultDivExpression:
I think the "correct" or logical precedence of operators is:
(strongest): Power of (x^y)
logical operators (x<y)
multiplication/division
additon/ substraction
So the following formula:
3+2>1*4<2^3
should be parsed as
3+((2>1)*(4<(2^3)))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the other hand, if we look at other programming languages, e.g. JavaScript, the logical operators have the lowest precedence:
What do you think, what would be an appropriate handling / precedence? I would recommend to implement the general consensus, as in other languages...
OK, I have decided that your solution and your implementation of the operator precedence is correct. I will merge your contribution into my develop branch, add some documentation and examples, and release a new version. many thanks for your useful contribution. |
Thanks for reviewing the PR so thoroughly. I am glad that, in the end, it worked out as intended! |
This PR adds the possibility of having logical operators and expressions in a formula.
Possible operators are
>
,<
,>=
,<=
,=
and!=
, the meaning of which is trivial.Since the final goal is to use those operators in a mathematical formula, the result of a logical expression will always return a number: either
1
(when the expression istrue
) or0
(when it isfalse
), which can easily be used in a formula to achieve conditional calculations.An example of a logical expression used in a formula is:
This would be essential to implement some logical functions – such as
IF()
– in the same form Microsoft Excel or Google Sheets do: