+, -, *, /, () expression calculator

In an effort to understand how compilers work, I wrote a simple expression calculator in C#. A CalculatorExpression takes an infix string, converts the infix string to postfix, and finally takes the postfix to an internal BinaryExpression tree representation.

To use, simply create an expression, and then evaluate:

var exp = new CalculatorExpression("1 + (1 + 2*3)"); 
double val = exp.Value;

I'm interested in having my use of access modifiers and general code structure, as well as readability (do I have too many comments/documentation?), reviewed.

First, here is the core code for the tree representation:

```
///
/// Represents a binary operation between two doubles
///
internal delegate double Operator(double x, double y);

///
/// Represents a node in BinaryExpression tree
///
internal class BinaryExpression
{
protected BinaryExpression()
{
}

///
/// Constructs a BinaryExpression tree from left and right subtrees
/// to be combined with an operator
///
public BinaryExpression(BinaryExpression left, BinaryExpression right,
Operator op)
{
Left = left; Right = right; Operator = op;
}

///
/// Returns the value of the tree
///
public virtual double Value
{
get
{
return Operator(Left.Value, Right.Value);
}
protected set { } // only child classes (i.e. BinaryAtomic) should be able to set Value
}

public BinaryExpression Left;
public BinaryExpression Right;
public Operator Operator;
}

///
/// Represents a leaf in BinaryExpression tree
///
internal class BinaryAtomic : BinaryExpression
{
protected BinaryAtomic()
{
}

///
/// Constructs a leaf
///
public BinaryAtomic(double value)
{
Value = value;
}

///
/// Returns the leaf value
///
public override double Value
{
get;
protected set;
}

public override string ToString

Your code has a pretty massive flaw - it only accepts single digit numbers.

Trying to use new CalculatorExpression("10+1") falls over (specifically your assertion fails). A calculator that can only handle single digit numbers isn't really that useful.

I'd recommend you changing the signature of your ToPostFix method to return an IEnumerable where each string is a token. You could actually create a token class if you want.

So you might have

ToPostFix("10+1");
// ["10", "1", "+"]

Then your parsing code can consume each logical bit at a time.

As I mentioned in my comment, I'm fairly confident this is an implementation of the shunting yard algorith which you may find interesting to read up about.

I never seem to have enough time, there's a lot I'd like to say but I'll limit myself in the interest of at least saying something:

private static bool IsOperator(string op)
{
    return op == "-" || op == "+" ||
           op == "*" || op == "/";
}

Could be rewritten as:

private static bool IsOperator(string op)
{
    return Operators.ContainsKey(op);
}

I don't like the name BinaryAtomic for two reasons:

• It's not actaully a binary expression

• This doesn't relate to atomicity

A better heirarchy to use would be:

abstract class Expression
{
    // Some factory methods?

    abstract double GetValue();
}

class BinaryExpression : Expression
{
    Operator Operator { get; set; }
    Expression Left { get; set; }
    Expression Right { get; set; }

    override double GetValue() 
    {
        return Operator(Left.GetValue(), Right.GetValue());
    }
}

class NumberExpression : Expression 
{
    double Value { get; set; }

    override double GetValue() 
    {
        return Value;
    }
}

... Time's up, hopefully I'll get a chance to write some more later.

I should point out that your probably don't want to use Expression as the name as it will walk all over the System.Linq.Expressions namespace. I couldn't think of anything better quickly.

StackExchange Code Review Q#93997, answer score: 12