Problems in evaluating postfix

[P4R4N01D]

I have got a fully working function that converts infix to postfix, but now I am having trouble w/ the second stage in evaluating the equation in postfix. I realise that there is an example given in K&R but I don't really understand it & have decided to make my own from scratch.
I had this algorithm working yesterday, but I changed it as it was extracting from the wrong end and couldn't parse equations like: 1 2 - 4 5 + *. The problem I am having is:
given the input 1 2 -, it returns 1 instead of -1;
given the input 666 1 -, it returns 65 instead of 665;
given the input 1+2, it returns nothing instead of 3 etc.
I am sure the function performoperation is working correctly & i have also checked that evaluation is correct for the above inputs after the strncpy function is called. I have not changed anything in what calls this function (& also this function is called on a button-click even so I cannot post it here).
No idea as to the cause of this problem (I was going really well until this prob arose) Please Help!

//INP: equation OUT: evaluation
char *EvaluateEquation(char *equation, char *evaluation) {
    int op1, op2, signbit = 0;
    int nextToken = 0;
    char *writeoperation;
    while (*equation != '\0') {
        if (isdigit(*equation)) {  
            while (isdigit(*equation) || *equation == '.') {                     
                *evaluation++ = *equation++; //Store number to evaluation
                if (!nextToken) nextToken = 1; //Space Seperate each operand
            }
        }
        else if (CheckIfOperator(*equation)) { 
            op1 = 0;
            op2 = 0;
            evaluation -= 2;
            /*Read operands in reverse order starting from the end */     
            //keep extracting until a ' ' is found & store the number
            do {
                if (*evaluation == '-') signbit = 1;         
                else {
                    op2 = op2 * 10 + (*evaluation - '0');
                    if (signbit) {
                        op2 = -op2;
                        signbit = 0; 
                    }  
                }
                evaluation--;
            } while (*evaluation != ' ');
            evaluation--; //Skip the space
            //keep extracting until another ' ' is found & store number
            while (*evaluation != ' ' && *evaluation != '\0') {
                if (*evaluation == '-') signbit = 1;         
                else {
                    op1 = op1 * 10 + (*evaluation - '0');
                    if (signbit) {
                        op1 = -op1;
                        signbit = 0; 
                    }  
                }
                evaluation--;
            }
            writeoperation = performoperation(op1, op2, *equation);  //Works fine upto and including this line
             
            if (writeoperation == NULL){ //Check for DB0
                strcpy(evaluation, "ERROR: Division by Zero");
                return evaluation;
            }
            strncpy(evaluation, writeoperation, sizeof(writeoperation));
            while (*evaluation != ' ') { // ' ' at the end of the operand
                evaluation++;
            }
            evaluation++; //we shouldn't have removed the space
            /* the operation has been performed so set the reset address to
              the next element, as the operation cannot be overwritten by
              an operand. All remains of operands MUST be removed from
              evaluation before continuing*/
            while (*evaluation != '\0') *evaluation++ = '\0';
            /*Note: if second operator not found, evaluation = evalcopy
              then evaluation-- till *evaluation == ' ' */
            equation++;
        }                  
        else if (*equation == ' ') equation++;
        //if nextToken add space to evaluation
        if (nextToken) { //Space-seperate each token, this helps w/ multiple
            *evaluation++ = ' '; //digits
            nextToken = 0;
        }
    }
    equation++;
}

[zigga15]

I wrote one of these for uni a few years ago, wish I could find the code for you. I do remember that it was a lot shorter than your implementation though.

The biggest thing that leaps out to me about this code is that it is all one function. Debatabley, anything over around 50 lines is too complex.

You should write a function that parses the strings into arrays, or some other data structure. you will find it easier to work with.

After that its simply a matter of having a temp variable for the "number_so_far" and a case statement which performs calculations depending on which operator it encounters.

[P4R4N01D]

This way i get to learn more about pointers . Also I have to output the string as a char *.

[tabstop]

So your "check backward until you see a space" can't possibly work, because there's no space in front of the first number.

[zigga15]

Learning pointers is one thing, learning to decompose a problem well is another. But all that aside, there are a lot of corner cases in postfix operations that you need to consider. And like tabstop noticed, if you want to read it backwards you will have to take into account the first character read. Hence this is why you should modularize your code, it will be prettier, and if you ever want a job in the software engineering field you will want good programming practices.

Also, you should be able to convert from one data type to another. just write another function which parses it back :P

This is how I learnt pointers - anyone reading this should give it a look :P

http://www.youtube.com/watch?v=r4-9Mh3761E

[P4R4N01D]

Both op1 & op2 work & they are the only two times I check backward, it is like a digit is being cutoff from equation.
Also I have tried to modularise the code by calling two other functions (it would be alot longer w/ performoperation()'s switch inside this function).

EDIT Here is a better version, filloperands() works as expected so I am not going to include it

//INP: equation OUT: evaluation
char *EvaluateEquation(char *equation, char *evaluation) {
    int op1, op2, signbit = 0;
    int nextToken = 0;
    char *writeoperation;
    while (*equation != '\0') {
        if (isdigit(*equation)) {  
            while (isdigit(*equation) || *equation == '.') {                     
                *evaluation++ = *equation++; //Store number to evaluation
                if (!nextToken) nextToken = 1; //Space Seperate each operand
            }
        }
        else if (CheckIfOperator(*equation)) { 
            op1 = 0;
            op2 = 0;
            evaluation -= 2;
            /*Read operands in reverse order starting from the end */     
            //keep extracting until a ' ' is found & store the number
            do {
                filloperands(&op2, *evaluation);
                evaluation--;
            } while (*evaluation != ' ');
            evaluation--; //Skip the space
            //keep extracting until another ' ' is found & store number
            while (*evaluation != ' ' && *evaluation != '\0') {
                filloperands(&op1, *evaluation);
                evaluation--;
            }
            writeoperation = performoperation(op1, op2, *equation);  //Works fine upto and including this line
             
            if (writeoperation == NULL){ //Check for DB0
                strcpy(evaluation, "ERROR: Division by Zero");
                return evaluation;
            }
            strncpy(evaluation, writeoperation, sizeof(writeoperation));
            while (*evaluation != ' ') { // ' ' at the end of the operand
                evaluation++;
            }
            evaluation++; //we shouldn't have removed the space
            /* the operation has been performed so set the reset address to
              the next element, as the operation cannot be overwritten by
              an operand. All remains of operands MUST be removed from
              evaluation before continuing*/
            while (*evaluation != '\0') *evaluation++ = '\0';
            /*Note: if second operator not found, evaluation = evalcopy
              then evaluation-- till *evaluation == ' ' */
            equation++;
        }                  
        else if (*equation == ' ') equation++;
        //if nextToken add space to evaluation
        if (nextToken) { //Space-seperate each token, this helps w/ multiple
            *evaluation++ = ' '; //digits
            nextToken = 0;
        }
    }
    equation++;
}

[tabstop]

If you want to affect the integer that op points to, you have to use *op.

[P4R4N01D]

fixed, posted modified code above. Still having the same problem as I described in the first post though.
Found what could be the problem, something is stuffed w/ equation & it goes back & does the isdigit() part after it has done the operation w/ the input 1 2 -.

[P4R4N01D]

This might fix the problem, performoperation() alters equation so the function is posted below:

char *performoperation(int op1, int op2, char chroperator) {
    char *Returnoperation;
    int operation;
    switch (chroperator) {
        case '+': { 
            operation = op1 + op2;
            break;
        }
        case '-': {
             operation = op1 - op2;
            break;
        }    
        case '*': {
             operation = op1 * op2;
            break;
        }    
        case '/': {
            if (!op2) return NULL;
            operation = op1 / op2;
            break;
        }
        case '%': {
            operation = op1 % op2;
            break;
        }
    }
    sprintf(Returnoperation, "%d ", operation);
    return Returnoperation;
}

[tabstop]

Looking some more, this seems odd:

 strncpy(evaluation, writeoperation, sizeof(writeoperation));

presumably you mean strlen(writeoperation). And does writeoperation lose the first character of the answer? Who knows? And aren't you overwriting something important here, should someone be so foolish as to type in more than one operation? (It looks like you keep writing \0 until the end of the string.)

[P4R4N01D]

I intended it to be strncpy as, say 1 2 is in evaluation. These are the operands. after an oparation is finished, the operands are no longer needed so evaluation can become -1 (if the operator was '-'
). Thanks for pointing that out, I meant strlen. writeoperation does not loose part of its answer.
WOW! that's better, 1 2 - now equals " 2", a slight improvement.

[tabstop]

Do me a favor, because I'm a naturally skeptic person. Add this line to the beginning of your function:

char *original_evaluation = evaluation;

and change this:

            //keep extracting until another ' ' is found & store number
            while (*evaluation != ' ' && *evaluation != '\0') {

to this:

            //keep extracting until another ' ' is found & store number
            while (*evaluation != ' ' && evaluation > original_evaluation) {

'cause I still think you're walking off the end of that string there.

If that doesn't help, post/attach the whole thing, so that I don't have to write a lot of code to go around all this.

[P4R4N01D]

Thanks, that fixed it! After that loop, I needed to call filloperands again as op2 was not being read. Thanks for your help tabstop and zigga15. I liked that youtube clip btw, not quite what I meant but anyway. I was thinking more of the push & pop operations on char *s. It's using the stack & these structures that I intended to get more familiar w/. I understand the basics of *s, I just haven't used them for long enough.

Still, back to the same problem I had yesterday, The output is incorrect for expressions like 1 2 - 4 5 + * and for 1 2 - 4 +. I will post the wole code below:

int CheckIfOperator(char c) {
    char *listofoperators = "+-*/%";
    int found = 0;
    while ((*listofoperators != '\0') && (!found)) {
          if (*listofoperators == c) found = 1;
          listofoperators++;
    }
    return found;
}

void filloperands(int *op, char evaluation) {
    int signbit = 0; 
    if (evaluation == '-') signbit = 1;         
    *op = *op * 10 + (evaluation - '0');
    if (signbit) {
        *op = -*op;
        signbit = 0; 
    }
    return;
}

//INP: equation OUT: evaluation
void EvaluateEquation(char *equation, char *evaluation) {
    int op1, op2, nextToken = 0;
    char *evalcopy;
    char *original_evaluation = evaluation;
    evalcopy = evaluation; //Just for debug
    char *writeoperation;
    while (*equation != '\0') {
        if (isdigit(*equation)) {  
            while (isdigit(*equation) || *equation == '.') {                     
                *evaluation++ = *equation++; //Store number to evaluation
                if (!nextToken) nextToken = 1; //Space Seperate each operand
            }
        }
        else if (*equation == ' ') equation++;
        else if (CheckIfOperator(*equation)) { 
            op1 = 0;
            op2 = 0;
            evaluation -= 2;
            /*Read operands in reverse order starting from the end */     
            //keep extracting until a ' ' is found & store the number
            do {
                filloperands(&op2, *evaluation);
                evaluation--;
            } while (*evaluation != ' ' && evaluation > original_evaluation);
            evaluation--; //Skip the space
            
            //keep extracting until another ' ' is found & store number
            while (*evaluation != ' ' && evaluation > original_evaluation) {
                filloperands(&op1, *evaluation);
                evaluation--;
            }
            filloperands(&op1, *evaluation);
            writeoperation = performoperation(op1, op2, *equation);
             
            if (writeoperation == NULL){ //Check for DB0
                strcpy(evaluation, "ERROR: Division by Zero");
                return;
            }
            strncpy(evaluation, writeoperation, strlen(writeoperation));
            /* the operation has been performed so set the reset address to
              the next element, as the operation cannot be overwritten by
              an operand. All remains of operands MUST be removed from
              evaluation before continuing*/
            /*Note: if second operator not found, evaluation = evalcopy
              then evaluation-- till *evaluation == ' ' */
            equation++;
        }                  
        //if nextToken add space to evaluation
        if (nextToken) { //Space-seperate each token, this helps w/ multiple
            *evaluation++ = ' '; //digits
            nextToken = 0;
        }
    }
    equation++;
}

char *performoperation(int op1, int op2, char chroperator) {
    char *Returnoperation;
    int operation;
    switch (chroperator) {
        case '+': { 
            operation = op1 + op2;
            break;
        }
        case '-': {
             operation = op1 - op2;
            break;
        }    
        case '*': {
             operation = op1 * op2;
            break;
        }    
        case '/': {
            if (!op2) return NULL;
            operation = op1 / op2;
            break;
        }
        case '%': {
            operation = op1 % op2;
            break;
        }
    }
    sprintf(Returnoperation, "%d ", operation);
    return Returnoperation;
}

[tabstop]

Your EvaluateEquation doesn't return anything. What is it supposed to return, and when?

[P4R4N01D]

woops, it alters evaluation, so effectively that is the return value. The function should be declared as void EvaluateEquation(char *equation, char *evaluation)