There are a few things about the design which could be improved - I'd get rid of the global variables
p, top, save, temp
top
should be a private data member of the class
stack
. All the others are temporary variables which can be declared locally within the scope where they are required to be used.
The use of ASCII codes such as 48, 57 would be more clearly understood as the characters '0' and '9' while this line
|
if (x[i] >= 42 && x[i] <= 47)
|
is a poor compromise, since it accepts all of these characters: "*+,-./" but it should only allow "*+-/".
I'd suggest changing that to
|
if ( strchr("+-*/", x[i]) )
|
see
http://www.cplusplus.com/reference/cstring/strchr/
As for the more serious errors in the code, the
pop()
function is flawed in more than one way.
27 28 29 30 31 32 33 34 35 36
|
char pop(){
if (top == NULL){
cout<<"Empty!!";
}else{
temp = top;
top = top->next;
return(temp->data);
delete temp;
}
}
|
1. The
node
contains data of type
int
but the function is returning a
char
.
2. Control may reach the end of the function at line 36 without returning any value. It must always return
something.
3. The return statement at line 33 exits from the function. That means the
delete
at line 34 can never be executed. That gives a memory leak - memory allocated which can never be released.
Something like this could work:
27 28 29 30 31 32 33 34 35 36 37 38 39
|
int pop() // return an integer
{
if (top)
{
node * temp = top;
top = top->next;
int data = temp->data;
delete temp;
return data;
}
cout << "Empty!!";
return 0; // must return something!
}
|
You may find the following thread on a related topic of interest.
http://www.cplusplus.com/forum/general/203063/