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

Expression design

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

Problem

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

Solution


  • 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.



  • 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".



  • 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.

Context

StackExchange Code Review Q#83536, answer score: 5

Revisions (0)

No revisions yet.