Infix to postfix Converter implementation
Clash Royale CLAN TAG#URR8PPP
.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty margin-bottom:0;
up vote
1
down vote
favorite
Please suggest some ways to improve my code for calculating a postfix expression from an infix expression entered by the user. The size of the input (Infix
string) should not exceed 10 charaters since the maximum size of the input string mentioned in the code is 10.
#include<stdio.h>
#include<conio.h>
#include<ctype.h>
char Infix[10];
char Symbol[10];
char Postfix[10];
void Push();
void Pop();
int Preced(char, char);
void main()
clrscr();
printf("Enter a Infix Expression:");
scanf("%s",&Infix);
Push();
printf("nPostfix of Entered Expression: %s",Postfix);
getch();
void Push()
int i=0, j=0, k=0, s=0;
while(Infix[i]!='')
while(--j>=0)
Symbol[j]=='*'
int Preced(char c1, char c2)
c math-expression-eval
add a comment |Â
up vote
1
down vote
favorite
Please suggest some ways to improve my code for calculating a postfix expression from an infix expression entered by the user. The size of the input (Infix
string) should not exceed 10 charaters since the maximum size of the input string mentioned in the code is 10.
#include<stdio.h>
#include<conio.h>
#include<ctype.h>
char Infix[10];
char Symbol[10];
char Postfix[10];
void Push();
void Pop();
int Preced(char, char);
void main()
clrscr();
printf("Enter a Infix Expression:");
scanf("%s",&Infix);
Push();
printf("nPostfix of Entered Expression: %s",Postfix);
getch();
void Push()
int i=0, j=0, k=0, s=0;
while(Infix[i]!='')
while(--j>=0)
Symbol[j]=='*'
int Preced(char c1, char c2)
c math-expression-eval
add a comment |Â
up vote
1
down vote
favorite
up vote
1
down vote
favorite
Please suggest some ways to improve my code for calculating a postfix expression from an infix expression entered by the user. The size of the input (Infix
string) should not exceed 10 charaters since the maximum size of the input string mentioned in the code is 10.
#include<stdio.h>
#include<conio.h>
#include<ctype.h>
char Infix[10];
char Symbol[10];
char Postfix[10];
void Push();
void Pop();
int Preced(char, char);
void main()
clrscr();
printf("Enter a Infix Expression:");
scanf("%s",&Infix);
Push();
printf("nPostfix of Entered Expression: %s",Postfix);
getch();
void Push()
int i=0, j=0, k=0, s=0;
while(Infix[i]!='')
while(--j>=0)
Symbol[j]=='*'
int Preced(char c1, char c2)
c math-expression-eval
Please suggest some ways to improve my code for calculating a postfix expression from an infix expression entered by the user. The size of the input (Infix
string) should not exceed 10 charaters since the maximum size of the input string mentioned in the code is 10.
#include<stdio.h>
#include<conio.h>
#include<ctype.h>
char Infix[10];
char Symbol[10];
char Postfix[10];
void Push();
void Pop();
int Preced(char, char);
void main()
clrscr();
printf("Enter a Infix Expression:");
scanf("%s",&Infix);
Push();
printf("nPostfix of Entered Expression: %s",Postfix);
getch();
void Push()
int i=0, j=0, k=0, s=0;
while(Infix[i]!='')
while(--j>=0)
Symbol[j]=='*'
int Preced(char c1, char c2)
c math-expression-eval
edited Jan 12 at 19:21
200_success
123k14143401
123k14143401
asked Jan 12 at 13:50
Sarabjot Singh
212
212
add a comment |Â
add a comment |Â
1 Answer
1
active
oldest
votes
up vote
1
down vote
The intent of your preced
function is completely unclear. I can't make heads or tails of what it's supposed to be doing as a whole. It appears to return 1
for two operators of equal precedence, but it also returns 1 for some cases when they're not equal--but it's not at all clear what those cases are supposed to be.
As a general rule, my immediate reaction would be to encode operator precedence into a table, and then have a little bit of logic to deal directly with precedence it found for them. So, I'd probably start with some code just to get the precedence:
int get_precedence(char ch)
char const *operators =
"^",
"+-",
"*/%"
;
int i;
for (i=0; i<3; i++)
if (strchr(operators[i], ch) != NULL)
return i;
return -1;
Using this, the "return 1 if they're equal" part could look something like this:
int preced(char ch1, char ch2)
if (get_precedence(ch1) == get_precedence(ch2))
return 1;
I'm not quite sure about the rest of the logic, so I won't try to deal with that right now, but I'd almost be surprised if it didn't come out rather clearer after treatment like this as well.
That brings up an obvious second point: preced
isn't a particularly great name for the function. It hints at the fact that it's somehow related to precedence, but that's about all we get from the name. Something more descriptive of the real intent would clearly help a lot here.
Use of NULL
NULL is intended to be a null pointer constant--not anything else. It should never be assigned to anything other than a pointer. Right now, you have places where you use it as a char
instead, such as:
Symbol[j-1]=NULL;
Although it's possible for a compiler to define NULL
in a way that allows this, it's also possible to define NULL
so you'd have to add a cast for this to work, so this affects both style/readability and portability.
Avoid arbitrary limits
If at all reasonable, and I think it probably is, it's generally best to avoid having entirely arbitrary limits on things like the size of the input. If you are going to have an arbitrary limit, at least try to make it large enough that it's unlikely to ever arise in practice (e.g., set it to 8 kilobytes instead of 10 bytes).
Enforce the limits you do have
Any time you see scanf("%s", somestring);
you should cringe in fear, bordering on horror. You've just used scanf
to re-create gets
, a function so terrible it's been removed from recent versions of the standard.
The problem here is that although your string is (in this case) only 10 bytes long, you haven't informed scanf
of that fact, so if the user types in more than 9 characters (remember, the string needs a NUL terminator) you'll get undefined behavior. Any time you use the %s
conversion with scanf
(or cousins like fscanf
) you need to include a maximum size allowed:
char buffer[1025];
scanf("%1024s", buffer);
So here we've specified a maximum length of 1024, with a buffer length of 1025, so we have room for the NUL terminator. Result: the code is nice and safe. No matter how much data the user enters, scanf
will never write past the end of our buffer.
add a comment |Â
1 Answer
1
active
oldest
votes
1 Answer
1
active
oldest
votes
active
oldest
votes
active
oldest
votes
up vote
1
down vote
The intent of your preced
function is completely unclear. I can't make heads or tails of what it's supposed to be doing as a whole. It appears to return 1
for two operators of equal precedence, but it also returns 1 for some cases when they're not equal--but it's not at all clear what those cases are supposed to be.
As a general rule, my immediate reaction would be to encode operator precedence into a table, and then have a little bit of logic to deal directly with precedence it found for them. So, I'd probably start with some code just to get the precedence:
int get_precedence(char ch)
char const *operators =
"^",
"+-",
"*/%"
;
int i;
for (i=0; i<3; i++)
if (strchr(operators[i], ch) != NULL)
return i;
return -1;
Using this, the "return 1 if they're equal" part could look something like this:
int preced(char ch1, char ch2)
if (get_precedence(ch1) == get_precedence(ch2))
return 1;
I'm not quite sure about the rest of the logic, so I won't try to deal with that right now, but I'd almost be surprised if it didn't come out rather clearer after treatment like this as well.
That brings up an obvious second point: preced
isn't a particularly great name for the function. It hints at the fact that it's somehow related to precedence, but that's about all we get from the name. Something more descriptive of the real intent would clearly help a lot here.
Use of NULL
NULL is intended to be a null pointer constant--not anything else. It should never be assigned to anything other than a pointer. Right now, you have places where you use it as a char
instead, such as:
Symbol[j-1]=NULL;
Although it's possible for a compiler to define NULL
in a way that allows this, it's also possible to define NULL
so you'd have to add a cast for this to work, so this affects both style/readability and portability.
Avoid arbitrary limits
If at all reasonable, and I think it probably is, it's generally best to avoid having entirely arbitrary limits on things like the size of the input. If you are going to have an arbitrary limit, at least try to make it large enough that it's unlikely to ever arise in practice (e.g., set it to 8 kilobytes instead of 10 bytes).
Enforce the limits you do have
Any time you see scanf("%s", somestring);
you should cringe in fear, bordering on horror. You've just used scanf
to re-create gets
, a function so terrible it's been removed from recent versions of the standard.
The problem here is that although your string is (in this case) only 10 bytes long, you haven't informed scanf
of that fact, so if the user types in more than 9 characters (remember, the string needs a NUL terminator) you'll get undefined behavior. Any time you use the %s
conversion with scanf
(or cousins like fscanf
) you need to include a maximum size allowed:
char buffer[1025];
scanf("%1024s", buffer);
So here we've specified a maximum length of 1024, with a buffer length of 1025, so we have room for the NUL terminator. Result: the code is nice and safe. No matter how much data the user enters, scanf
will never write past the end of our buffer.
add a comment |Â
up vote
1
down vote
The intent of your preced
function is completely unclear. I can't make heads or tails of what it's supposed to be doing as a whole. It appears to return 1
for two operators of equal precedence, but it also returns 1 for some cases when they're not equal--but it's not at all clear what those cases are supposed to be.
As a general rule, my immediate reaction would be to encode operator precedence into a table, and then have a little bit of logic to deal directly with precedence it found for them. So, I'd probably start with some code just to get the precedence:
int get_precedence(char ch)
char const *operators =
"^",
"+-",
"*/%"
;
int i;
for (i=0; i<3; i++)
if (strchr(operators[i], ch) != NULL)
return i;
return -1;
Using this, the "return 1 if they're equal" part could look something like this:
int preced(char ch1, char ch2)
if (get_precedence(ch1) == get_precedence(ch2))
return 1;
I'm not quite sure about the rest of the logic, so I won't try to deal with that right now, but I'd almost be surprised if it didn't come out rather clearer after treatment like this as well.
That brings up an obvious second point: preced
isn't a particularly great name for the function. It hints at the fact that it's somehow related to precedence, but that's about all we get from the name. Something more descriptive of the real intent would clearly help a lot here.
Use of NULL
NULL is intended to be a null pointer constant--not anything else. It should never be assigned to anything other than a pointer. Right now, you have places where you use it as a char
instead, such as:
Symbol[j-1]=NULL;
Although it's possible for a compiler to define NULL
in a way that allows this, it's also possible to define NULL
so you'd have to add a cast for this to work, so this affects both style/readability and portability.
Avoid arbitrary limits
If at all reasonable, and I think it probably is, it's generally best to avoid having entirely arbitrary limits on things like the size of the input. If you are going to have an arbitrary limit, at least try to make it large enough that it's unlikely to ever arise in practice (e.g., set it to 8 kilobytes instead of 10 bytes).
Enforce the limits you do have
Any time you see scanf("%s", somestring);
you should cringe in fear, bordering on horror. You've just used scanf
to re-create gets
, a function so terrible it's been removed from recent versions of the standard.
The problem here is that although your string is (in this case) only 10 bytes long, you haven't informed scanf
of that fact, so if the user types in more than 9 characters (remember, the string needs a NUL terminator) you'll get undefined behavior. Any time you use the %s
conversion with scanf
(or cousins like fscanf
) you need to include a maximum size allowed:
char buffer[1025];
scanf("%1024s", buffer);
So here we've specified a maximum length of 1024, with a buffer length of 1025, so we have room for the NUL terminator. Result: the code is nice and safe. No matter how much data the user enters, scanf
will never write past the end of our buffer.
add a comment |Â
up vote
1
down vote
up vote
1
down vote
The intent of your preced
function is completely unclear. I can't make heads or tails of what it's supposed to be doing as a whole. It appears to return 1
for two operators of equal precedence, but it also returns 1 for some cases when they're not equal--but it's not at all clear what those cases are supposed to be.
As a general rule, my immediate reaction would be to encode operator precedence into a table, and then have a little bit of logic to deal directly with precedence it found for them. So, I'd probably start with some code just to get the precedence:
int get_precedence(char ch)
char const *operators =
"^",
"+-",
"*/%"
;
int i;
for (i=0; i<3; i++)
if (strchr(operators[i], ch) != NULL)
return i;
return -1;
Using this, the "return 1 if they're equal" part could look something like this:
int preced(char ch1, char ch2)
if (get_precedence(ch1) == get_precedence(ch2))
return 1;
I'm not quite sure about the rest of the logic, so I won't try to deal with that right now, but I'd almost be surprised if it didn't come out rather clearer after treatment like this as well.
That brings up an obvious second point: preced
isn't a particularly great name for the function. It hints at the fact that it's somehow related to precedence, but that's about all we get from the name. Something more descriptive of the real intent would clearly help a lot here.
Use of NULL
NULL is intended to be a null pointer constant--not anything else. It should never be assigned to anything other than a pointer. Right now, you have places where you use it as a char
instead, such as:
Symbol[j-1]=NULL;
Although it's possible for a compiler to define NULL
in a way that allows this, it's also possible to define NULL
so you'd have to add a cast for this to work, so this affects both style/readability and portability.
Avoid arbitrary limits
If at all reasonable, and I think it probably is, it's generally best to avoid having entirely arbitrary limits on things like the size of the input. If you are going to have an arbitrary limit, at least try to make it large enough that it's unlikely to ever arise in practice (e.g., set it to 8 kilobytes instead of 10 bytes).
Enforce the limits you do have
Any time you see scanf("%s", somestring);
you should cringe in fear, bordering on horror. You've just used scanf
to re-create gets
, a function so terrible it's been removed from recent versions of the standard.
The problem here is that although your string is (in this case) only 10 bytes long, you haven't informed scanf
of that fact, so if the user types in more than 9 characters (remember, the string needs a NUL terminator) you'll get undefined behavior. Any time you use the %s
conversion with scanf
(or cousins like fscanf
) you need to include a maximum size allowed:
char buffer[1025];
scanf("%1024s", buffer);
So here we've specified a maximum length of 1024, with a buffer length of 1025, so we have room for the NUL terminator. Result: the code is nice and safe. No matter how much data the user enters, scanf
will never write past the end of our buffer.
The intent of your preced
function is completely unclear. I can't make heads or tails of what it's supposed to be doing as a whole. It appears to return 1
for two operators of equal precedence, but it also returns 1 for some cases when they're not equal--but it's not at all clear what those cases are supposed to be.
As a general rule, my immediate reaction would be to encode operator precedence into a table, and then have a little bit of logic to deal directly with precedence it found for them. So, I'd probably start with some code just to get the precedence:
int get_precedence(char ch)
char const *operators =
"^",
"+-",
"*/%"
;
int i;
for (i=0; i<3; i++)
if (strchr(operators[i], ch) != NULL)
return i;
return -1;
Using this, the "return 1 if they're equal" part could look something like this:
int preced(char ch1, char ch2)
if (get_precedence(ch1) == get_precedence(ch2))
return 1;
I'm not quite sure about the rest of the logic, so I won't try to deal with that right now, but I'd almost be surprised if it didn't come out rather clearer after treatment like this as well.
That brings up an obvious second point: preced
isn't a particularly great name for the function. It hints at the fact that it's somehow related to precedence, but that's about all we get from the name. Something more descriptive of the real intent would clearly help a lot here.
Use of NULL
NULL is intended to be a null pointer constant--not anything else. It should never be assigned to anything other than a pointer. Right now, you have places where you use it as a char
instead, such as:
Symbol[j-1]=NULL;
Although it's possible for a compiler to define NULL
in a way that allows this, it's also possible to define NULL
so you'd have to add a cast for this to work, so this affects both style/readability and portability.
Avoid arbitrary limits
If at all reasonable, and I think it probably is, it's generally best to avoid having entirely arbitrary limits on things like the size of the input. If you are going to have an arbitrary limit, at least try to make it large enough that it's unlikely to ever arise in practice (e.g., set it to 8 kilobytes instead of 10 bytes).
Enforce the limits you do have
Any time you see scanf("%s", somestring);
you should cringe in fear, bordering on horror. You've just used scanf
to re-create gets
, a function so terrible it's been removed from recent versions of the standard.
The problem here is that although your string is (in this case) only 10 bytes long, you haven't informed scanf
of that fact, so if the user types in more than 9 characters (remember, the string needs a NUL terminator) you'll get undefined behavior. Any time you use the %s
conversion with scanf
(or cousins like fscanf
) you need to include a maximum size allowed:
char buffer[1025];
scanf("%1024s", buffer);
So here we've specified a maximum length of 1024, with a buffer length of 1025, so we have room for the NUL terminator. Result: the code is nice and safe. No matter how much data the user enters, scanf
will never write past the end of our buffer.
answered Jan 12 at 19:03
Jerry Coffin
27.4k360123
27.4k360123
add a comment |Â
add a comment |Â
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f184956%2finfix-to-postfix-converter-implementation%23new-answer', 'question_page');
);
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password