Infix to postfix Converter implementation

The name of the pictureThe name of the pictureThe name of the pictureClash 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)







share|improve this question



























    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)







    share|improve this question























      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)







      share|improve this question













      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)









      share|improve this question












      share|improve this question




      share|improve this question








      edited Jan 12 at 19:21









      200_success

      123k14143401




      123k14143401









      asked Jan 12 at 13:50









      Sarabjot Singh

      212




      212




















          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.






          share|improve this answer





















            Your Answer




            StackExchange.ifUsing("editor", function ()
            return StackExchange.using("mathjaxEditing", function ()
            StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix)
            StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
            );
            );
            , "mathjax-editing");

            StackExchange.ifUsing("editor", function ()
            StackExchange.using("externalEditor", function ()
            StackExchange.using("snippets", function ()
            StackExchange.snippets.init();
            );
            );
            , "code-snippets");

            StackExchange.ready(function()
            var channelOptions =
            tags: "".split(" "),
            id: "196"
            ;
            initTagRenderer("".split(" "), "".split(" "), channelOptions);

            StackExchange.using("externalEditor", function()
            // Have to fire editor after snippets, if snippets enabled
            if (StackExchange.settings.snippets.snippetsEnabled)
            StackExchange.using("snippets", function()
            createEditor();
            );

            else
            createEditor();

            );

            function createEditor()
            StackExchange.prepareEditor(
            heartbeatType: 'answer',
            convertImagesToLinks: false,
            noModals: false,
            showLowRepImageUploadWarning: true,
            reputationToPostImages: null,
            bindNavPrevention: true,
            postfix: "",
            onDemand: true,
            discardSelector: ".discard-answer"
            ,immediatelyShowMarkdownHelp:true
            );



            );








             

            draft saved


            draft discarded


















            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






























            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.






            share|improve this answer

























              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.






              share|improve this answer























                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.






                share|improve this answer













                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.







                share|improve this answer













                share|improve this answer



                share|improve this answer











                answered Jan 12 at 19:03









                Jerry Coffin

                27.4k360123




                27.4k360123






















                     

                    draft saved


                    draft discarded


























                     


                    draft saved


                    draft discarded














                    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













































































                    Popular posts from this blog

                    Chat program with C++ and SFML

                    Function to Return a JSON Like Objects Using VBA Collections and Arrays

                    Will my employers contract hold up in court?