Calculating a value of a simple calculating math's string.

Hello, I meant with a simple calculating which just using the operators (+ - * / ) and entering the simple calculator's equation as a string.. e.g: "3+9/6+2" which the return must be the result of this simple math equation as the result in this case is, 4, (begin from the left to the end with ignorantly of the precedences) .
I've been struggling about a complete day -new programmer :( -and there's no errors but just getting incorrect result's answer , I checked/reviewed it more than once and I dont know why it's not working fine though all functions go well!!
can anyone help me or actually gimme a hint for modifying and fix my code to work correctly in proper way?

here is my code:

Code:

---

#include<stdio.h>
#include <string.h>
#include<stdlib.h>
double evaluate(char expr[]);
int main()
{
  char expr[21];
  double ssum=0;
  printf("Input ur string:\n");
  scanf("%20s",&expr);
  sssum = evaluate(expr);
  printf("Output: %.2lf", sssum);
  return 0;
}
 
 
double evaluate(char expr[])//this function for evaluating the value of the four equation's calculation//
{


    char n='r';//doesn't matter what's the string because the code itself pushes it immediately after getting a new string//
    int i=0,j=0;
    float temp2=0,temp=0;
    while (i<strlen(expr))
    {
        while (1)//this function for obtaining the whole number(complete number) until we arrive to any operators of +-*///
        {
            temp=temp*10+(expr[i]-'0');
            i+=1;
        if (expr[i]=='+' || expr[i]=='/' || expr[i]=='*'|| expr[i]=='-') break;
        }
        switch (expr[i])//after getting out of the second while, the code will proceed to swtich function)
        {
        case '+':// by the way temp2 is the collecting of all the values in all cases which at the end the final answer included in it//
            {
            n='+';
            temp2+=temp;
            temp=0;
            break;
            }
LINE2:
        case '-':
            n='-';
            if(temp2==0)
            {
                    temp2=temp-temp2;
                    temp=0;
                    break;
            }
            else
            {
                    temp2=temp2-temp;
                    temp=0;
                    break;
            }
LINE3:
        case '*':
            n='*';
            if ((expr[0]-'0')!=0)
            {
                temp2=1;
                temp2=temp2*temp;
                temp=0;
                break;
            }
            else
            {
                temp2=temp2*temp;
                temp=0;
                break;
            }
        case '/':
            temp2=1;
            temp2/=temp;
            temp=0;
            break;
        default:
                temp2+=temp;
                break;
        }
        i+=1;
    }
    return temp2;
}

---

I tired hard to find what's the trouble in my code, unfortunately didn't figure out, it's almost done but need some finishes and modifications so please would be appreciated if you help me for. thanks!!

I got an error while compiling your code, because you're declaring a variable "ssum" but using it as "sssum". What the heck is that supposed to mean? You should be giving all of your variables meaningful names.

When I fixed that, I got the following warnings:

Code:

---

/*
main.c||In function 'main':|
main.c|10|warning: format '%20s' expects type 'char *', but argument 2 has type 'char (*)[21]'|
main.c||In function 'evaluate':|
main.c|56|warning: label 'LINE3' defined but not used|
main.c|41|warning: label 'LINE2' defined but not used|
main.c|22|warning: unused variable 'j'|
||=== Build finished: 0 errors, 4 warnings ===|
*/

---

The last one is not a big deal.

The two before that are a bit strange - what's with the labels? They aren't needed for anything.

The first means you should not have the '&' in your "scanf()" call on line 10. You need to provide a pointer to the variable that will store the scanned input ... but an array name by itself acts as a pointer to its first element.

Code:

---

char expr[21];

// ...

scanf("%20s",expr);

---

Next, I have a complaint that might make me seem like a cantankerous old man. This line:

Code:

---

printf("Input ur string:\n");

---

The word is "your", not "ur". This is a computer program, not a text message. You are free to use whatever language you want in your programs, of course ... but I am also free to voice my opinion when I see something that irks me personally.

You should strive for clarity when programming a machine that interfaces with other human beings. Especially when it threatens to make you look like a fool in front of others. Serious programmers are professionals, and childish shorthand like this will not earn you points when it matters.

That was a bit harsh, and I do apologize for it. Especially since you so far seem to be taking advice given to you to heart. My only defense is, in the words of Al Stewart; "This is my nature ... to it, I have to be true".

Anyway, moving on.

Look at your "while(1)" loop. That is an infinite loop, so you need to take care of when to break out of it. So far, you only break out of it if the (next) character is an operator. (And I'd suggest checking for an operator first there, and not incrementing "i" in that loop unless you found a digit.)

But what happens when it reaches the last value? The last character is presumably a digit, and you only break out of the infinite loop when an operator is found. So you're not breaking out of the infinite loop at the end of the string ... and you keep going, accessing your "expr" array out of bounds.

Furthermore, your technique of building up an integer from a string using "expr[i]-'0" is generally alright ... but you're trying to store the results in a float. Building up a floating point number in this fashion is a totally different beast. If your input is only supposed to be integers, I would suggest declaring those variables as ints.

This will go a part of the way towards finding your problems, but there are still others to be had.

I would suggest you consider planning out the logic more before trying to write code. This would be a good time to learn flow-charts. You can plan the logic, step by step, by hand on paper, and verify (and understand) it before comitting it to code.

Quote:

---

Originally Posted by Matticus

I got an error while compiling your code, because you're declaring a variable "ssum" but using it as "sssum". What the heck is that supposed to mean? You should be giving all of your variables meaningful names.

When I fixed that, I got the following warnings:

Code:

---

/*
main.c||In function 'main':|
main.c|10|warning: format '%20s' expects type 'char *', but argument 2 has type 'char (*)[21]'|
main.c||In function 'evaluate':|
main.c|56|warning: label 'LINE3' defined but not used|
main.c|41|warning: label 'LINE2' defined but not used|
main.c|22|warning: unused variable 'j'|
||=== Build finished: 0 errors, 4 warnings ===|
*/

---

The last one is not a big deal.

The two before that are a bit strange - what's with the labels? They aren't needed for anything.

The first means you should not have the '&' in your "scanf()" call on line 10. You need to provide a pointer to the variable that will store the scanned input ... but an array name by itself acts as a pointer to its first element.

Code:

---

char expr[21];

// ...

scanf("%20s",expr);

---

Next, I have a complaint that might make me seem like a cantankerous old man. This line:

Code:

---

printf("Input ur string:\n");

---

The word is "your", not "ur". This is a computer program, not a text message. You are free to use whatever language you want in your programs, of course ... but I am also free to voice my opinion when I see something that irks me personally.

You should strive for clarity when programming a machine that interfaces with other human beings. Especially when it threatens to make you look like a fool in front of others. Serious programmers are professionals, and childish shorthand like this will not earn you points when it matters.

That was a bit harsh, and I do apologize for it. Especially since you so far seem to be taking advice given to you to heart. My only defense is, in the words of Al Stewart; "This is my nature ... to it, I have to be true".

Anyway, moving on.

Look at your "while(1)" loop. That is an infinite loop, so you need to take care of when to break out of it. So far, you only break out of it if the (next) character is an operator. (And I'd suggest checking for an operator first there, and not incrementing "i" in that loop unless you found a digit.)

But what happens when it reaches the last value? The last character is presumably a digit, and you only break out of the infinite loop when an operator is found. So you're not breaking out of the infinite loop at the end of the string ... and you keep going, accessing your "expr" array out of bounds.

Furthermore, your technique of building up an integer from a string using "expr[i]-'0" is generally alright ... but you're trying to store the results in a float. Building up a floating point number in this fashion is a totally different beast. If your input is only supposed to be integers, I would suggest declaring those variables as ints.

This will go a part of the way towards finding your problems, but there are still others to be had.

I would suggest you consider planning out the logic more before trying to write code. This would be a good time to learn flow-charts. You can plan the logic, step by step, by hand on paper, and verify (and understand) it before comitting it to code.

---

I thought in another logic way at all which is inserting/splitting at first the string math in every encounters of any operators (+-*/) and afterwards calculating the split strings... would be more useful and efficient but I actually in this way dont know from where to begin and hasn't much information on splitting strings.

Good advice from Matticus.

You're on the right track -- quite a few bugs and stuff to work out but it's the right idea.

On the subject of splitting strings.... I don't think that's the right thing to do right now. Given your example in your first post:

3+9/6+2

How would you want to split it up? I guess something like:

Code:

---

3    +9    /6    +2
12            /6    +2
2                    +2
4

---

That is, assuming you still want to ignore operator precedence.

If you look at that and think about it, hopefully you'll see a big thing wrong with your existing code. In that table, there's always a number on the left, which is progress with the calculations so far. It looks like you were trying to use temp2 for this --- the problem, I think, is that you are looping looking for number then operator. So you end up parsing 3+ and 9/, which is logically awkward.

I'd treat the first number (3) as special and get its value outside the main loop. Then, parse an operator then a number and apply it repeatedly during the loop. If you wanted to, you could split the input into smaller strings, but I don't think that'll actually gain you anything.

You do need to look out for the end of the string (a '\0' NUL terminator), and make sure that any loops for parsing numbers and operators are properly controlled. Like, your number parsing loop condition should be "while(expr[i] is a number char)".

Quote:

---

Originally Posted by smokeyangel

Good advice from Matticus.

You're on the right track -- quite a few bugs and stuff to work out but it's the right idea.

On the subject of splitting strings.... I don't think that's the right thing to do right now. Given your example in your first post:

3+9/6+2

How would you want to split it up? I guess something like:

Code:

---

3    +9    /6    +2
12            /6    +2
2                    +2
4

---

That is, assuming you still want to ignore operator precedence.

If you look at that and think about it, hopefully you'll see a big thing wrong with your existing code. In that table, there's always a number on the left, which is progress with the calculations so far. It looks like you were trying to use temp2 for this --- the problem, I think, is that you are looping looking for number then operator. So you end up parsing 3+ and 9/, which is logically awkward.

I'd treat the first number (3) as special and get its value outside the main loop. Then, parse an operator then a number and apply it repeatedly during the loop. If you wanted to, you could split the input into smaller strings, but I don't think that'll actually gain you anything.

You do need to look out for the end of the string (a '\0' NUL terminator), and make sure that any loops for parsing numbers and operators are properly controlled. Like, your number parsing loop condition should be "while(expr[i] is a number char)".

---

You are correct that splitting ...would be useless, may you show me or actually how could I modify my previous code?(that I posted it in the post), I didn't get your concept of, exactly.
thanks.

Ok. Since you're learning, I've kept it pretty similar to your code, so it doesn't throw a bunch of new concepts in. The resulting code is pretty fragile (will not work if you, for example, input a floating point number or put some spaces in the input).

I think I could have just reordered your existing code to make it work, but there were a few other things I wanted to address.

This is what I did with evaluate():
First we have a small loop just to read the first number into the value variable. This is outside of the main loop.

Then you can start your main loop. The loop will
- Read an operator
- Read a number into the operand variable
- Apply the operator, with value as the LHS and operand as the RHS.

I've renamed the variables as I think temp and temp2 were not very helpful.

Ok. Code!

Code:

---

float evaluate(char expr[])
{
  float value = 0; /* Partial result */
  float operand = 0;
  char op = '\0';

  int i = 0;

  /* Read the first number. */
  while (isdigit(expr[i])) {
      value=value*10+(expr[i]-'0'); 
      i++;
  }

  /* Main loop */
  while (i < strlen(expr)) {     
      /* Operand first */
      while (!(expr[i]=='+' || expr[i]=='/' || expr[i]=='*'|| expr[i]=='-')) {
        /* Check for NUL  */
        if (expr[i] == '\0') {
            return value;     
        }
        i++;
      }
     
      op = expr[i];
      i++;
       
      /* Read number */
      while (isdigit(expr[i])) {
        operand = operand *10+(expr[i]-'0'); 
        i++;
      }

      switch (op) {
      case '+':
        value += operand;
        break;
      case '-':
        value -= operand;
        break;
      case '/':
        value /= operand;
        break;
      case '*':
        value *= operand;
        break;
      }
     
      operand = 0;
  }
 
  return value; 
}

---

You might need to include <ctype.h> for isdigit().

This bit of code:

Code:

---

while (!(expr[i]=='+' || expr[i]=='/' || expr[i]=='*'|| expr[i]=='-')) {
        /* Check for NUL  */
        if (expr[i] == '\0') {

---

shouldn't be necessary with the current implementation and correct input. The strlen check will exit the loop.
I put it in mainly because I was worried about a newline character being stored after the string (fgets() does this, scanf(%s) doesn't). However, it's a good idea to check strlen or for '\0' in any loop that could theoretically run off into the distance unchecked.

Some thoughts on what's still to do:
- it'd be nice to allow whitespace in the input.
- the program should fail gracefully with an error on invalid input
- the number parsing code is repeated twice -- would be nice to put it in a separate function (that also updates "i")
- I'd probably get rid of "i" altogether and use a pointer to expr. Each time I processed something I'd increment the pointer, chomping off each number/op as I went.

Hope that helps!

Ahaa - I just saw your other thread. My previous reply was really just about getting this code working, and won't help with operator precedence at all.

It might help a bit with tokenisation though. There's an.... interesting looking way to directly calculate results from infix notation here: Operator-precedence parser - Wikipedia, the free encyclopedia and plenty of more informative bits around the web.

So for something like that, you do indeed want to split the string up first. I saw another thread with some funky ways of doing that, but in the end you can just examine it character by character.

You need to decide on a way to store your substrings/tokens. Certainly an array of tokens -- but how you represent the tokens is up to you. You could have an array of char arrays, and store them as strings. Or calculate the numeric values of the numbers and store as ints -- but then the operators would be indistinguishable, so you'd need to store that information too. And then there's all the info about the operators associativity, precedence, etc.... which would probably go in a lookup table.

Then on to actually implementing the algorithm....

Then on to debugging a big pile of recursion.

I wouldn't want to take this on as a beginner. I guess practice makes perfect, but you'd have to be pretty determined.

Quote:

---

Originally Posted by smokeyangel

Ok. Since you're learning, I've kept it pretty similar to your code, so it doesn't throw a bunch of new concepts in. The resulting code is pretty fragile (will not work if you, for example, input a floating point number or put some spaces in the input).

I think I could have just reordered your existing code to make it work, but there were a few other things I wanted to address.

This is what I did with evaluate():
First we have a small loop just to read the first number into the value variable. This is outside of the main loop.

Then you can start your main loop. The loop will
- Read an operator
- Read a number into the operand variable
- Apply the operator, with value as the LHS and operand as the RHS.

I've renamed the variables as I think temp and temp2 were not very helpful.

Ok. Code!

Code:

---

float evaluate(char expr[])
{
  float value = 0; /* Partial result */
  float operand = 0;
  char op = '\0';

  int i = 0;

  /* Read the first number. */
  while (isdigit(expr[i])) {
      value=value*10+(expr[i]-'0'); 
      i++;
  }

  /* Main loop */
  while (i < strlen(expr)) {     
      /* Operand first */
      while (!(expr[i]=='+' || expr[i]=='/' || expr[i]=='*'|| expr[i]=='-')) {
        /* Check for NUL  */
        if (expr[i] == '\0') {
            return value;     
        }
        i++;
      }
     
      op = expr[i];
      i++;
       
      /* Read number */
      while (isdigit(expr[i])) {
        operand = operand *10+(expr[i]-'0'); 
        i++;
      }

      switch (op) {
      case '+':
        value += operand;
        break;
      case '-':
        value -= operand;
        break;
      case '/':
        value /= operand;
        break;
      case '*':
        value *= operand;
        break;
      }
     
      operand = 0;
  }
 
  return value; 
}

---

You might need to include <ctype.h> for isdigit().

This bit of code:

Code:

---

while (!(expr[i]=='+' || expr[i]=='/' || expr[i]=='*'|| expr[i]=='-')) {
        /* Check for NUL  */
        if (expr[i] == '\0') {

---

shouldn't be necessary with the current implementation and correct input. The strlen check will exit the loop.
I put it in mainly because I was worried about a newline character being stored after the string (fgets() does this, scanf(%s) doesn't). However, it's a good idea to check strlen or for '\0' in any loop that could theoretically run off into the distance unchecked.

Some thoughts on what's still to do:
- it'd be nice to allow whitespace in the input.
- the program should fail gracefully with an error on invalid input
- the number parsing code is repeated twice -- would be nice to put it in a separate function (that also updates "i")
- I'd probably get rid of "i" altogether and use a pointer to expr. Each time I processed something I'd increment the pointer, chomping off each number/op as I went.

Hope that helps!

---

Yes,the code's logic that you've written is in the same direction I've implemented above in my code, and if you don't mind I've a lil question about specific function in ur code which is :
what does this code below(see below)stand for? what's his role in? I didn't understand it very well...may you elaborate by an e.g?
the code is :

Code:

---

while(!(expr[i]=='+'|| expr[i]=='/'|| expr[i]=='*'|| expr[i]=='-')) {        /* Check for NUL  */
        if(expr[i] == '\0')
            return value;{

---

I know it mainly doesn't necessary in this code and you've cleared out about;in last ur post. but I'm so curious to know what would this function do and exactly how it works because it's really weird for me.

thanks beforehand.

I've decided it shouldn't be there, but I'll explain what I was thinking anyway:

Initially my version of the program let you put whitespace in the expression, like "1 + 2 + 3". scanf would be a pain about that, stopping at every space so I changed it to fgets(), which read a line of stdin, including whitespace, into a buffer.

The other difference is that fgets stores the newline from when you pressed Enter, so you'd get "1 + 2 + 3\n", whereas scanf leaves it waiting on stdin to potentially screw up the time the user is asked for input (since it'll have a bonus leading newline).

For this and a bunch of other reasons (google "scanf is evil"...), I prefer to use fgets for line reads then do whatever parsing is needed after the read.


So, these possibilities exist for the code you're asking about.

The first is that the code isn't run, because strlen matches i.

2. Input of "1 +1" (no newline, but whitespace)
3. Input of "1+1\n" (newline from fgets)


The while loop is run when expr[i] is NOT an operator (note the ! at the start of the condition). This could mean harmless whitespace, or could mean we've got a trailing newline. Or both. Or something completely different -- I forgot to check that the non-operator character were whitespace.

C strings are all terminated with a byte of 0, aka NUL or '\0'.
So the while loop loops, incrementing i and checking to see if we have reached the end of the string. If we have, then all the previous stuff has already been processed, so we return value.
If it is just harmless whitespace in the middle of the expression, then we move up to the operator and carry on.

I had a similiar "if !number" loop to allow spaces there too, but removed that.


Writing about it, I've decided that it's a bad way to do it. Better would be to enforce that evaluate() takes a string with only numbers and operators -- by processing the input before calling evaluate().