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;