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

Parser for simple programming language

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

Problem

I'm writing a basic LL(1) parser in java, but my parser class is quickly getting out of hand and becoming huge. PMD even calls it a "God class" and says that it has "Too many methods." Is there a way that I can refactor this class?

```
public class Parser {
private static final APValueNum NEGATIVE_ONE = new APValueNum(
new BigDecimal("-1"));
LinkedList tokens = new LinkedList<>();
Token lookahead;

public Parser(final List tokens) {
this.tokens.addAll(tokens);
if (tokens.isEmpty()) {
throw new ParserException("Cannot parse an empty file!");
}
}

private void nextToken() {
try {
tokens.pop();
} catch (final NoSuchElementException e) {
throw new ParserException("Ran out of characters!", e);
}
// at the end of input we return an epsilon token
if (tokens.isEmpty()) {
lookahead = new Token(TokenType.EOF, "");
} else {
lookahead = tokens.getFirst();
}
}

public List parse(final Context context) {

final List expressions = new ArrayList<>();

lookahead = tokens.getFirst();

while (lookahead.getType() != TokenType.EOF) {
expressions.add(statement(context));
}
return expressions;
}

private ExpressionNode statement(final Context context) {
final ExpressionNode.VariableNode expr = identifier();
nextToken();
if (lookahead.getType() == TokenType.EQUAL) {
final ExpressionNode assignment = assignment(context, expr);
return assignment;
} else if (lookahead.getType() == TokenType.IDENTIFIER) {
// we have a parameter
nextToken();
if (lookahead.getType() == TokenType.IDENTIFIER
|| lookahead.getType() == TokenType.EQUAL) {
final List variables = new ArrayList<>();
while (lookahead.getType() == TokenType

Solution

When I look at this code, I find an immediate criticism that may help. You name many of your functions as nouns, e.g.

private ExpressionNode statement(final Context context) {
private ExpressionNode assignment(final Context context,
        final ExpressionNode.VariableNode expr) {
private ExpressionNode expression(final Context context) {
private ExpressionNode ifExpr(final Context context) {


Are you sure that these should be functions? If you are naming a bunch of nouns, maybe these should be classes, each of which could have its own parse or getNext method. This is even true of

private void nextToken() {


But the solution is probably different. Perhaps simply rename that to getNextLookahead(). Alternately, you could make a new TokenStream class with a getNext function, or a higher level ExpressionNode class like Package, Program, or File with a parse method.

Of course, if you follow this advice, you will end up with a much larger number of small classes that basically exist to wrap the parse method in each. You will probably have to pass your Parser to each the way that you already pass a Context.

Code Snippets

private ExpressionNode statement(final Context context) {
private ExpressionNode assignment(final Context context,
        final ExpressionNode.VariableNode expr) {
private ExpressionNode expression(final Context context) {
private ExpressionNode ifExpr(final Context context) {
private void nextToken() {

Context

StackExchange Code Review Q#68448, answer score: 4

Revisions (0)

No revisions yet.