Expression design

Question

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};
    }
}

Answer 1

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.

Answer 2

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;