4
\$\begingroup\$

I'm designing a Tokenizer / Parser / AST. I'm wondering if this basic idea of an expression is good and if this way of implementing Simplification is good, or should I use something like the Visitor pattern? Is implementing it in the class itself a good idea?

public abstract class Expr
{
    public Expr Left;
    public Expr Right;

    public abstract bool CanSimplify { get; set; } 

    public abstract Expr Simplify();
}

public class ConstExpr : Expr
{
    public double Value;

    public override bool CanSimplify { get; set; } = false;

    public override Expr Simplify()
    {
        return this;
    }

    public override string ToString()
    {
        return Value.ToString(CultureInfo.InvariantCulture);
    }
}

public class AddExpr : Expr
{
    public override bool CanSimplify { get; set; } = true;
    public override Expr Simplify()
    {
        while (Left.CanSimplify)
            Left.Simplify();
        while (Right.CanSimplify)
            Right.Simplify();

        var left = Left as ConstExpr;
        var right = Right as ConstExpr;
        if (left == null || right == null)
            return this;
        return new ConstExpr {Value = left.Value + right.Value};
    }
}
\$\endgroup\$
5
\$\begingroup\$
  1. You have a bug in your code: Simplify() can return a new instance, and you ignore this instance when calling Left.Simplify() and Right.Simplify(). Also, I do not see the reason to iterate on simplification (while (Left.CanSimplify)), as it appears as each expression will only need one call to be simplified.
  2. Expr class exposes 2 public fields, Left and Right. Exposing public fields is a bad practice, and I would assume there may be cases where expression can have 1 or more than 2 "parameters".
  3. Can you make use of Expression Trees available in .NET instead of developing your own framework? It seems like a good fit for your purpose. If not - I would still consider copying design patterns from it, like immutability of expressions and Visitor pattern for walking the expression tree.
\$\endgroup\$
4
\$\begingroup\$
public abstract bool CanSimplify { get; set; } 
public override bool CanSimplify { get; set; } = false;

This property shouldn't have a setter, especially not a public one. Instead, since you seem to be using C# 6.0, you can use expression-bodied properties:

public abstract bool CanSimplify { get; }
public override bool CanSimplify => false;
\$\endgroup\$

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.