views:

61

answers:

6
public override Models.CalculationNode Parse(string expression)
{
    var calNode = new Models.CalculationNode();

    int i = expression.Length;
    char[] x = expression.ToCharArray();
    string temp = "";

    //Backwards assembly of the tree

    //Right Node
    while (!IsOperator(x[i]) && i > 0)
    {
        if (!x[i].Equals(' ')) temp = x[i] + temp;
        i--;
    }
}

It has been a while since I've used trees and I'm getting an out of bounds exception in the while loop.

+1  A: 

character array is from zero to length-1

Steven A. Lowe
+1  A: 

You should try writing int i = x.Length - 1;.

As soon as x contains items indexed from 0 to x.Length - 1, x[expression.Length] seems to be just one item out of bounds.

Li0liQ
+1  A: 

I'd reverse the test:

while (i >= 0 && !IsOperator(x[i]))

because the IsOperator will be evaluated first and i will be -1 at the end of the loop (not withstanding any problems you might have with the start of the loop).

ChrisF
+1  A: 

You've got an off-by-1 error when you start at i = expression.Length. That first index will be out of bounds right off the bat. You could rewrite the loop as a for loop like so:

char[] x = expression.ToCharArray();
string temp = "";

//Backwards assembly of the tree

//Right Node
for (int i = x.Length - 1; i >= 0 && !IsOperator(x[i]); --i)
{
    if (!x[i].Equals(' ')) temp = x[i] + temp;
}
John Kugelman
thanks this is much cleaner
Matt
A: 

You need:

int i = expression.Length;

and then in the while loop you will need:

while (!IsOperator(x[i]) && i >= 0)

Arrays are 0 based, so 0 is the first position and the final position is the length minus 1.

cfeduke
A: 

You're setting i to be the length of the string, which starts at 1. Your array indexing starts at 0 though, so when you're accessing the element at the end, you're actually trying to go 1 beyond your bounds. It's the first run of the loop that's throwing the error.

You need to add -1 to your initialisation of i.

AaronM