Skip to content
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.

Sign up for GitHub

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

Code for issues 56 and 57, including tests #58

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Code for issues 56 and 57, including tests #58

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions src/main/java/net/objecthunter/exp4j/ExpressionBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import net.objecthunter.exp4j.function.Functions;
import net.objecthunter.exp4j.operator.Operator;
import net.objecthunter.exp4j.shuntingyard.ShuntingYard;
import net.objecthunter.exp4j.tokenizer.Tokenizer;

/**
* Factory class for {@link Expression} instances. This class is the main API entrypoint. Users should create new
Expand Down Expand Up @@ -111,10 +112,15 @@ public ExpressionBuilder operator(Operator operator) {
return this;
}

private Tokenizer operatorChecker = null;

private void checkOperatorSymbol(Operator op) {
if (operatorChecker == null) {
operatorChecker = createTokenizer();
}
String name = op.getSymbol();
for (char ch : name.toCharArray()) {
if (!Operator.isAllowedOperatorChar(ch)) {
if (! operatorChecker.isAllowedOperatorChar(ch)) {
throw new IllegalArgumentException("The operator symbol '" + name + "' is invalid");
}
}
Expand Down Expand Up @@ -163,8 +169,12 @@ public Expression build() {
throw new IllegalArgumentException("A variable can not have the same name as a function [" + var + "]");
}
}
return new Expression(ShuntingYard.convertToRPN(this.expression, this.userFunctions, this.userOperators, this.variableNames),
return new Expression(ShuntingYard.convertToRPN(this.expression, this.userFunctions, this.userOperators, this.variableNames, createTokenizer()),
this.userFunctions.keySet());
}

protected Tokenizer createTokenizer() {
return new Tokenizer(expression, userFunctions, userOperators, variableNames);
}

}
19 changes: 0 additions & 19 deletions src/main/java/net/objecthunter/exp4j/operator/Operator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,6 @@ public abstract class Operator {
*/
public static final int PRECEDENCE_UNARY_PLUS = PRECEDENCE_UNARY_MINUS;

/**
* The set of allowed operator chars
*/
public static final char[] ALLOWED_OPERATOR_CHARS = { '+', '-', '*', '/', '%', '^', '!', '#','§', '$', '&', ';', ':', '~', '<', '>', '|', '='};

protected final int numOperands;
protected final boolean leftAssociative;
protected final String symbol;
Expand All @@ -78,20 +73,6 @@ public Operator(String symbol, int numberOfOperands, boolean leftAssociative,
this.precedence = precedence;
}

/**
* Check if a character is an allowed operator char
* @param ch the char to check
* @return true if the char is allowed an an operator symbol, false otherwise
*/
public static boolean isAllowedOperatorChar(char ch) {
for (char allowed: ALLOWED_OPERATOR_CHARS) {
if (ch == allowed) {
return true;
}
}
return false;
}

/**
* Check if the operator is left associative
* @return true os the operator is left associative, false otherwise
Expand Down
3 changes: 1 addition & 2 deletions src/main/java/net/objecthunter/exp4j/shuntingyard/ShuntingYard.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,10 @@ public class ShuntingYard {
* @return a {@link net.objecthunter.exp4j.tokenizer.Token} array containing the result
*/
public static Token[] convertToRPN(final String expression, final Map<String, Function> userFunctions,
final Map<String, Operator> userOperators, final Set<String> variableNames){
final Map<String, Operator> userOperators, final Set<String> variableNames, Tokenizer tokenizer){
final Stack<Token> stack = new Stack<Token>();
final List<Token> output = new ArrayList<Token>();

final Tokenizer tokenizer = new Tokenizer(expression, userFunctions, userOperators, variableNames);
while (tokenizer.hasNext()) {
Token token = tokenizer.nextToken();
switch (token.getType()) {
Expand Down
39 changes: 35 additions & 4 deletions src/main/java/net/objecthunter/exp4j/tokenizer/Tokenizer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
*/
package net.objecthunter.exp4j.tokenizer;

import java.util.ArrayList;
import java.util.Collection;
import java.util.Map;
import java.util.Set;

Expand Down Expand Up @@ -86,9 +88,9 @@ public Token nextToken(){
return parseParentheses(true);
} else if (isCloseParentheses(ch)) {
return parseParentheses(false);
} else if (Operator.isAllowedOperatorChar(ch)) {
} else if (isAllowedOperatorChar(ch)) {
return parseOperatorToken(ch);
} else if (isAlphabetic(ch) || ch == '_') {
} else if (isVariableOrFunctionStartChar(ch)) {
// parse the name which can be a setVariable or a function
if (lastToken != null &&
(lastToken.getType() != Token.TOKEN_OPERATOR
Expand All @@ -105,7 +107,20 @@ public Token nextToken(){
throw new IllegalArgumentException("Unable to parse char '" + ch + "' (Code:" + (int) ch + ") at [" + pos + "]");
}

private Token parseArgumentSeparatorToken(char ch) {
protected boolean isVariableOrFunctionStartChar(char ch) {
return isAlphabetic(ch) || ch == '_';
}

/**
* The set of allowed operator chars
*/
public static final String DEFAULT_ALLOWED_OPERATOR_CHARS = "+-*/%^!#§$&;:~<>|=";

public boolean isAllowedOperatorChar(char ch) {
return DEFAULT_ALLOWED_OPERATOR_CHARS.indexOf(ch) >= 0;
}

private Token parseArgumentSeparatorToken(char ch) {
this.pos++;
this.lastToken = new ArgumentSeparatorToken();
return lastToken;
Expand Down Expand Up @@ -133,6 +148,16 @@ private boolean isCloseParentheses(char ch) {
return ch == ')' || ch == '}' || ch == ']';
}

private Collection<String> undefinedVariables = null;

public void allowUndefinedVariables(boolean allow) {
this.undefinedVariables = (allow ? new ArrayList<String>() : null);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you could use HashSet, or rather LinkedHashSet if you want to preserve tokens order. ArrayList will consume less memory but HashSet will perform better. I'm not sure what is more important here.

The ArrayList:

  • contains - O(N)
  • add - O(1)

The HashSet or LinkedHashSet:

  • contains - O(1)
  • add - O(1)

But also, with HashSet you could not check contains(name) since Set guarantees Strings uniqueness on add(name) so the number of operations is smaller.

Copy link
Author

Choose a reason for hiding this comment

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

ArrayList's add can be O(N), if it needs to reallocate the underlying array. Anyway, it's reasonable to expect the set of unknown variables to be small, so I don't think it really matters.

}

public String[] getUndefinedVariables() {
return (undefinedVariables != null ? undefinedVariables.toArray(new String[undefinedVariables.size()]) : null);
Copy link
Contributor

Choose a reason for hiding this comment

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

The conclusion after reading Arrays of Wisdom of the Ancients is that we should use toArray(new T[0]) instead of toArray(new T[size]):

Bottom line: toArray(new T[0]) seems faster, safer, and contractually cleaner, and therefore should be the default choice now. Future VM optimizations may close this performance gap for toArray(new T[size]), rendering the current "believed to be optimal" usages on par with an actually optimal one.

Copy link
Author

Choose a reason for hiding this comment

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

This I didn't know, I always assumed allocating the right size was best! Thanks for the "wisdom"!

}

private Token parseFunctionOrVariable() {
final int offset = this.pos;
int lastValidLen = 1;
Expand All @@ -154,6 +179,12 @@ private Token parseFunctionOrVariable() {
if (f != null) {
lastValidLen = len;
lastValidToken = new FunctionToken(f);
} else if (undefinedVariables != null) {
if (! undefinedVariables.contains(name)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

With HashSet this could be simply undefinedVariables.add(name) without checking undefinedVariables.contains(name).

Copy link
Author

Choose a reason for hiding this comment

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

If undefinedVariables is declared Collection the check should be there, since we cannot assume the check needn't be done. If it is declared Set, we can omit the check. I usually don't care about these kind of optimisations, for sets that are small.

undefinedVariables.add(name);
}
lastValidLen = len;
lastValidToken = new VariableToken(name);
}
}
len++;
Expand Down Expand Up @@ -184,7 +215,7 @@ private Token parseOperatorToken(char firstChar) {
Operator lastValid = null;
symbol.append(firstChar);

while (!isEndOfExpression(offset + len) && Operator.isAllowedOperatorChar(expression[offset + len])) {
while (!isEndOfExpression(offset + len) && isAllowedOperatorChar(expression[offset + len])) {
symbol.append(expression[offset + len++]);
}

Expand Down
25 changes: 16 additions & 9 deletions src/test/java/net/objecthunter/exp4j/shuntingyard/ShuntingYardTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,12 @@
import java.util.HashMap;
import java.util.HashSet;
import java.util.Map;
import java.util.Set;

import net.objecthunter.exp4j.function.Function;
import net.objecthunter.exp4j.operator.Operator;
import net.objecthunter.exp4j.tokenizer.Token;
import net.objecthunter.exp4j.tokenizer.Tokenizer;

import org.junit.Test;

Expand All @@ -32,16 +35,20 @@ public class ShuntingYardTest {
@Test
public void testShuntingYard1() throws Exception {
String expression = "2+3";
Token[] tokens = ShuntingYard.convertToRPN(expression, null, null, null);
Token[] tokens = convertToRPN(expression, null, null, null);
assertNumberToken(tokens[0], 2d);
assertNumberToken(tokens[1], 3d);
assertOperatorToken(tokens[2], "+", 2, Operator.PRECEDENCE_ADDITION);
}

protected <T, R> Token[] convertToRPN(String expression, Map<String, Function> userFunctions, Map<String, Operator> userOperators, Set<String> variableNames) {
return ShuntingYard.convertToRPN(expression, userFunctions, userOperators, variableNames, new Tokenizer(expression, userFunctions, userOperators, variableNames));
}

@Test
public void testShuntingYard2() throws Exception {
String expression = "3*x";
Token[] tokens = ShuntingYard.convertToRPN(expression, null, null, new HashSet<String>(Arrays.asList("x")));
Token[] tokens = convertToRPN(expression, null, null, new HashSet<String>(Arrays.asList("x")));
assertNumberToken(tokens[0], 3d);
assertVariableToken(tokens[1], "x");
assertOperatorToken(tokens[2], "*", 2, Operator.PRECEDENCE_MULTIPLICATION);
Expand All @@ -50,15 +57,15 @@ public void testShuntingYard2() throws Exception {
@Test
public void testShuntingYard3() throws Exception {
String expression = "-3";
Token[] tokens = ShuntingYard.convertToRPN(expression, null, null, null);
Token[] tokens = convertToRPN(expression, null, null, null);
assertNumberToken(tokens[0], 3d);
assertOperatorToken(tokens[1], "-", 1, Operator.PRECEDENCE_UNARY_MINUS);
}

@Test
public void testShuntingYard4() throws Exception {
String expression = "-2^2";
Token[] tokens = ShuntingYard.convertToRPN(expression, null, null, null);
Token[] tokens = convertToRPN(expression, null, null, null);
assertNumberToken(tokens[0], 2d);
assertNumberToken(tokens[1], 2d);
assertOperatorToken(tokens[2], "^", 2, Operator.PRECEDENCE_POWER);
Expand All @@ -68,7 +75,7 @@ public void testShuntingYard4() throws Exception {
@Test
public void testShuntingYard5() throws Exception {
String expression = "2^-2";
Token[] tokens = ShuntingYard.convertToRPN(expression, null, null, null);
Token[] tokens = convertToRPN(expression, null, null, null);
assertNumberToken(tokens[0], 2d);
assertNumberToken(tokens[1], 2d);
assertOperatorToken(tokens[2], "-", 1, Operator.PRECEDENCE_UNARY_MINUS);
Expand All @@ -77,7 +84,7 @@ public void testShuntingYard5() throws Exception {
@Test
public void testShuntingYard6() throws Exception {
String expression = "2^---+2";
Token[] tokens = ShuntingYard.convertToRPN(expression, null, null, null);
Token[] tokens = convertToRPN(expression, null, null, null);
assertNumberToken(tokens[0], 2d);
assertNumberToken(tokens[1], 2d);
assertOperatorToken(tokens[2], "+", 1, Operator.PRECEDENCE_UNARY_PLUS);
Expand Down Expand Up @@ -109,7 +116,7 @@ public double apply(double... args) {
};
Map<String, Operator> userOperators = new HashMap<String, Operator>();
userOperators.put("!", factorial);
Token[] tokens = ShuntingYard.convertToRPN(expression, null, userOperators, null);
Token[] tokens = convertToRPN(expression, null, userOperators, null);
assertNumberToken(tokens[0], 2d);
assertNumberToken(tokens[1], 2d);
assertOperatorToken(tokens[2], "!", 1, Operator.PRECEDENCE_POWER + 1);
Expand All @@ -120,7 +127,7 @@ public double apply(double... args) {
@Test
public void testShuntingYard8() throws Exception {
String expression = "-3^2";
Token[] tokens = ShuntingYard.convertToRPN(expression, null, null, null);
Token[] tokens = convertToRPN(expression, null, null, null);
assertNumberToken(tokens[0], 3d);
assertNumberToken(tokens[1], 2d);
assertOperatorToken(tokens[2], "^", 2, Operator.PRECEDENCE_POWER);
Expand All @@ -140,7 +147,7 @@ public double apply(final double... args) {
};
Map<String, Operator> userOperators = new HashMap<String, Operator>();
userOperators.put("$", reciprocal);
Token[] tokens = ShuntingYard.convertToRPN("1$", null, userOperators, null);
Token[] tokens = convertToRPN("1$", null, userOperators, null);
assertNumberToken(tokens[0], 1d);
assertOperatorToken(tokens[1], "$", 1, Operator.PRECEDENCE_DIVISION);
}
Expand Down
48 changes: 48 additions & 0 deletions src/test/java/net/objecthunter/exp4j/tokenizer/TokenizerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import static org.junit.Assert.*;

import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Map;
Expand Down Expand Up @@ -551,4 +552,51 @@ public void testTokenization22() throws Exception {

assertFalse(tokenizer.hasNext());
}

@Test
public void testAllowUndefinedVariablesFalse() {
final Tokenizer tokenizer = new Tokenizer("a * b", null, null, Collections.singleton("a"));
tokenizer.allowUndefinedVariables(false);

assertTrue(tokenizer.hasNext());
Token tok1 = tokenizer.nextToken();
assertTrue(tok1 instanceof VariableToken);
assertEquals("a", ((VariableToken) tok1).getName());
String[] undefinedVariables = tokenizer.getUndefinedVariables();
assertFalse(undefinedVariables != null && undefinedVariables.length > 0);

assertTrue(tokenizer.hasNext());
assertOperatorToken(tokenizer.nextToken(), "*", 2, Operator.PRECEDENCE_MULTIPLICATION);

assertTrue(tokenizer.hasNext());
try {
tokenizer.nextToken();
fail();
} catch (Exception e) {
assertTrue(e instanceof IllegalArgumentException);
}
}

@Test
public void testAllowUndefinedVariablesTrue() {
final Tokenizer tokenizer = new Tokenizer("a * b", null, null, Collections.singleton("a"));
tokenizer.allowUndefinedVariables(true);

assertTrue(tokenizer.hasNext());
Token tok1 = tokenizer.nextToken();
assertTrue(tok1 instanceof VariableToken);
assertEquals("a", ((VariableToken) tok1).getName());
assertEquals(0, tokenizer.getUndefinedVariables().length);

assertTrue(tokenizer.hasNext());
assertOperatorToken(tokenizer.nextToken(), "*", 2, Operator.PRECEDENCE_MULTIPLICATION);

assertTrue(tokenizer.hasNext());
Token tok3 = tokenizer.nextToken();
assertTrue(tok3 instanceof VariableToken);
assertEquals("b", ((VariableToken) tok3).getName());
String[] undefinedVariables = tokenizer.getUndefinedVariables();
assertEquals(1, undefinedVariables.length);
assertEquals("b", undefinedVariables[0]);
}
}