HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Parsing arithmetic expressions like (x + y * a + b *z)

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
likeparsingexpressionsarithmetic

Problem

I have this code which I wrote to parse arithmetic expressions.

However, many people say there are problems with it but don't tell me what the problem is. Please let me know if you see any.

Note that I know there is another way to do this. But I did it specifically this way as I am influenced by NLP parsing style and I wanted to do it the NLP style as well.

```
public class Parser {

private static final String P_CLOSE = ")";
private static final String P_OPEN = "(";

private Map frontCachingChart = new HashMap<>();
private Map backCachingChart = new HashMap<>();

// maps opener to closer
private Map closingMapper = new HashMap<>();
// maps closer to opener
private Map openingMapper = new HashMap<>();

private LinkedList opsList = new LinkedList<>();

public Expression parse(String input) {
init();

String org = input;
String[] tokens = extractTokens(input);

LinkedList stack = new LinkedList<>();
int depth = 0;
for (int i = 0; i opPriorityComparator = new Comparator() {
@Override
public int compare(Op o1, Op o2) {
// we want high priority op type first
return o2.type.priority - o1.type.priority;
}
};
// make sure higher priority op types are looked at first
Collections.sort(opsList, opPriorityComparator);

// make sure deeper ops are looked at first, the sort is stable so higher priority of same depth come first
// we will do bottom up parsing in this case to ease parsing left to right of expression like a + b + c + d
Collections.sort(opsList);

Expression ex = parse(tokens);
ex.org = org;
return ex;
}

private void init() {
frontCachingChart.clear();
backCachingChart.clear();
closingMapper.clear();
openingMapper.clear();
opsList.clear();
}

private String[] extractTokens(String input) {
input = input.replaceAll("\\(", " ( ");
input = input.replaceAll("\\)", " ) ");
input = input.replaceAll("\\+", " + ");
input = input.

Solution

Is this homework? Or just for fun? If it is for real work, I would suggest using an existing framework. I have not done parsing myself, but it is a common use-case and there are many frameworks. I vaguely know of antlr.

I am not a specialist in the domain, so I can't realy comment on your algorithm. But here are some general observations.

-
You can put the getOp method as a static method in the enum OpType. There is probably also an awful lot more code which you could put in this enum.

-
Initializing your member variables to -1 is a bit unusual. There is only one constructor which sets all of them, so the initialization is useless (in Op).

-
You should use getters/setters for class Expression. I know you don't actually need to, but it is the Java style. Besides, this library might grow at some point and you would likely extract Expression in its own file, where you would definitely need to add getters/setters.

-
Your methods are much too long. One method extraction which I noticed right away: at the start of parse(String input), you use about 10 lines to split the string, so put that in a method. Even inside for/while loops, you can extract some blocks of code as methods. The code becomes more readable that way.

-
You should think a bit more because I am certain you can make this much more OO. What screamed the need for OO to me was this:

private static Expression parse(String[] tokens, LinkedList opsList,
    Map closingMapper, Map openingMapper,
    Map frontCachingChart,
    Map backCachingChart )

Code Snippets

private static Expression parse(String[] tokens, LinkedList<Op> opsList,
    Map<Integer, Integer> closingMapper, Map<Integer, Integer> openingMapper,
    Map<Integer, Expression> frontCachingChart,
    Map<Integer, Expression> backCachingChart )

Context

StackExchange Code Review Q#56999, answer score: 5

Revisions (0)

No revisions yet.