We are building an interpreter for a language as part of the requirements in one of our undergraduate course. Part of the project involves writing a lexer and a parser. The parser generates an AST. Obviously, the AST would have nodes.

One of the nodes of that AST is the ArithmeticOperationNode. This node will be a node containing any operations involving arithmentic. The code below is a minimized version of the node.

package ds.ast;

import exceptions.CompilerException;
import exceptions.RuntimeException;

public class ArithmeticOperationNode extends OperationNode
    public ArithmeticOperationNode(String operation, Computable operator1, Computable operator2, int startingLine, int startingColumn)
        super(operation, operator1, operator2, startingLine, startingColumn);

    public Value compute() throws RuntimeException, CompilerException

NOTE: OperationNode implements an interface called Computable.

compute() will be responsible for performing the arithmetic operation between operator1 and operator2. operator1 and operator2 may be another arithmetic expression. The function will be called when we traverse the AST during execution. There are a lot of nodes that follow the same design, i.e. having a compute() function.

Am I violating the Single Responsibility Principle with this? I have a feeling that I am doing so.

  • 2
    I don't really see why...the primary purpose of AST nodes in an interpreter is to be interpreted, which for expressions would be to compute a value. This seems like a single responsibility. What do you see as conflicting responsibilities? – JacquesB May 12 '18 at 12:40
  • @JacquesB, do correct me on this. The way I see it is that, an "evaluator" would be the one that will execute the whole AST tree. The parser would just generate the AST. And lastly, the nodes will just hold data, and be, well, nodes. – Sean Francis N. Ballais May 12 '18 at 12:56

There do seem to be some conflation of responsibilites in that you have syntax level information (operation indicated as a string, line nr etc.) together with the semantic operation. I suspect compute() will end up having code similar to this (pseudocode):

if operation=="+":
  return a + b
if operation=="-":
  return a - b

Here the interpretation of the syntax is conflated with the semantics of the evaluation. The AST shouldn't really depend on the concrete syntax, which is why it is called an Abstract Syntax Tree. Instead the parser could generate AST classes like AddExpression, SubstractionExpression etc. which would only contain the semantic operation in compute() but not have any knowledge of the underlying syntax.

| improve this answer | |
  • Your suspicion is correct. It also has some type checking which I believe, should also be delegated somewhere else. So, I should just move the responsibility of compute() somewhere else, like what Martin Maat suggested? – Sean Francis N. Ballais May 12 '18 at 14:07
  • Essentially, making the node only contain data? Also, can you explain what "semantics of the evaluation" is? – Sean Francis N. Ballais May 12 '18 at 14:09
  • @SeanFrancisN.Ballais: No I would actually suggest the opposite, that compute() do belong in the AST class in an interpreter, but the concerns related to the syntax does not. By the semantics I mean the actual effect of the operation as opposed to the syntax. – JacquesB May 13 '18 at 1:12
  • After much thinking, I think I'll be placing the responsibilities of compute() into the class that will execute the whole AST. If I don't, I'd be creating unused "variables" but that's already outside the scope of this question and you might get confused. On the other hand, I will decompose my ArithmeticOperationNode into AddExpression and the like, just like you suggested. Thanks! – Sean Francis N. Ballais May 13 '18 at 8:21
  • 1
    I agree with everything Jacques said. Having a compute() methods in the AST is absolutely fine, especially if this is just a simple calculator language. But in my experience, using a separate Visitor that interprets the AST is preferable for more complicated languages, and makes it easier to manage bindings or complex control flow. Without a visitor, all the state that a visitor could hold would have to be passed as explicit params (compute(variableBindings, functions)) which doesn't waste anything, but gets increasingly inconvenient. Bindings/environments can simply be a HashMap. – amon May 13 '18 at 11:46

Yes you are. It may be because of your naming. You call your object ArithmeticOperation, already suggesting it does something. While at parse time it is still just an expression. Later in your question you do use that word (expression) which is really the better word in an AST context. You would not feel the need to add a Compute() method if the thing were called ArithmeticExpression.

I think the better route would be to recognize expressions rather than operations. You have two kinds: Unary and binary. As goes for operators: unary operators (like not) and binary operators (and, plus).

| improve this answer | |
  • Basing from your answer, I believe I should rename my ArithmeticOperation object to ArithmeticExpression, and delegate the task of computing for the expression to the module that will traverse through AST during execution. Correct? – Sean Francis N. Ballais May 12 '18 at 13:17
  • @Sean I think that would be better. The AST should just be a dissection of your code, it is not a processor. – Martin Maat May 12 '18 at 13:45
  • In other words, just let the damn tree be a tree? Haha. – Sean Francis N. Ballais May 12 '18 at 13:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.