Logic error: need advice

This is a discussion on Logic error: need advice within the C Programming forums, part of the General Programming Boards category; Hello.. I'm new in this forum.. I got two codes that I don't know what's the problems and I've try ...

  1. #1
    Registered User
    Join Date
    Oct 2012
    Posts
    11

    Unhappy Logic error: need advice

    Hello.. I'm new in this forum.. I got two codes that I don't know what's the problems and I've try so many but the result displayed is still the same.. forgive me for my bad english.. I'm from Malaysia.

    The first problem is about this code.

    Code:
    #include <stdio.h>
    #define MAXBUFFERSIZE   80
    int main(){
        
        int base;
        char optExit, userIn, optExitUpper;
        char buffer[MAXBUFFERSIZE]; //sufficient to handle one line
        int char_count; //number of characters read for this line
        
        do{ 
            system("cls");    
        
        opt:
            printf("\n\nBase: ");
            userIn = getchar();
            char_count = 0;
            
            while((userIn != '\n') && (char_count < MAXBUFFERSIZE)) { //avoid user from entering invalid input type
                        buffer[char_count++] = userIn;
                        userIn = getchar();
                    }
                    
           buffer[char_count] = 0x00; //null terminate buffer 
           base = atoi(buffer);//convert from char to int
           
            //example
            if (base == 1){
                
            }
            
            else {
                 printf ("Error!");
                 goto opt;
            }
                 
            //option to exit
            printf("\n\n\nRun application again (Y/N)?");
            scanf(" %c",&optExit);
            optExitUpper = toupper(optExit);
            
        } while (optExitUpper == 'Y');
    
        system("pause");
        return 0;
    }
    The first run is good, but when I choose to run the application again, the program automatically display 'Error!' after it display 'Base: '..

    Another question is about loop in the code below.

    Code:
    char* convBinToHex(int binNumInt){
       
       char binNumChar[9],hexNum[9];
       static char arrToStr[3];
       int i=0,j=0,temp;
       
       snprintf(binNumChar, sizeof(binNumChar), "%d", binNumInt); //convert integer to char
       
       while(binNumChar[i]){
          binNumChar[i] = binNumChar[i] -48;
          ++i;
       }
    
    
       --i;
       
       while(i-2>=0){
           temp =  binNumChar[i-3] *8 + binNumChar[i-2] *4 +  binNumChar[i-1] *2 + binNumChar[i] ;
           if(temp > 9)
               hexNum[j++] = temp + 55;
           else
                hexNum[j++] = temp + 48;
           i=i-4;
       }
    
    
       if(i ==1)
          hexNum[j] = binNumChar[i-1] *2 +binNumChar[i] + 48 ;
       else if(i==0)
          hexNum[j] =  binNumChar[i] + 48 ;
       else
          --j;
    
    
       strcpy(arrToStr,"");   
       while(j>=0){
          strcat(arrToStr,&hexNum[j--]); //combine all string
        }
        
        return arrToStr;
              
    }
    The last while loop didn't work perfectly. For example when I check the value of hexNum[j--] in every loops, it display FF or 3E but when I combine it and store it in arrToStr, it save as FFF or 3E3.

    How do I solve both problems? Thank you for all your help and advice.

  2. #2
    Registered User
    Join Date
    May 2010
    Posts
    2,560
    So much wrong but I'm going to concentrate on the first program. First goto?? I recommend you start by replacing the goto with one of the other looping constructs such as while(), do/while(). You are leaving the newline character in the input buffer with the last scanf() in your loop.

    Jim

  3. #3
    Registered User
    Join Date
    Oct 2012
    Posts
    11
    Quote Originally Posted by jimblumberg View Post
    So much wrong but I'm going to concentrate on the first program. First goto?? I recommend you start by replacing the goto with one of the other looping constructs such as while(), do/while(). You are leaving the newline character in the input buffer with the last scanf() in your loop.

    Jim
    Thank you for your quick reply.

    Actually, I don't have any other idea on how I can use the loop to cater the error while displaying the error message, that's why I used goto.

    How can I solve that input buffer?

  4. #4
    Registered User
    Join Date
    May 2010
    Posts
    2,560
    Actually, I don't have any other idea on how I can use the loop to cater the error while displaying the error message, that's why I used goto.
    You already have the code in a while loop. What if you were to reverse the logic of the following if statement?
    Code:
            //example
            if (base == 1){
                 
            }
             
            else {
                 printf ("Error!");
                 goto opt;
            }
                  
            //option to exit
            printf("\n\n\nRun application again (Y/N)?");
            scanf(" %c",&optExit);
            optExitUpper = toupper(optExit);
    To:
    Code:
            //example
            if (base != 1){
                 printf ("Error!");
            }
            else
            {
                //option to exit
                printf("\n\n\nRun application again (Y/N)?");
                scanf(" %c",&optExit);
                optExitUpper = toupper(optExit);
            }
    For the other problem you will need to extract that end of line character, one way would be to add a getchar() immediately after the problem scanf().


    Jim

  5. #5
    Registered User
    Join Date
    Oct 2012
    Posts
    11
    Thanks for the idea!!! I search through the forum about the input buffer and found some solutions, but yours is simpler!

    about the goto, actually for the selection statements, I have 4 selections and if the user entered wrong selection, error message will appear while for the message asking about whether the user wants to run the application again is after the program have bees used by the users. Here's the code. I'm sorry if I make you confused.

    Code:
    #include <stdio.h>
    #define MAXBUFFERSIZE   80
    int main(){
        
        int base;
        //int ch;
        char optExit, userIn, optExitUpper;
        char buffer[MAXBUFFERSIZE]; //sufficient to handle one line
        int char_count; //number of characters read for this line
        
        do{ 
            opt:
                printf("\n\nBase: ");
                userIn = getchar();
                char_count = 0;
                
                while((userIn != '\n') && (char_count < MAXBUFFERSIZE)) { //avoid user from entering invalid input type
                            buffer[char_count++] = userIn;
                            userIn = getchar();
                        }
                        
               buffer[char_count] = 0x00; //null terminate buffer 
               base = atoi(buffer);//convert from char to int
               
                //example
                if (base == 1){
                    
                }
                
                else if (base == 2){
                     
                }
                
                else if (base == 3){
                     
                }
                
                else if (base == 4){
                     
                }
    
    
                else {
                     printf ("Error!");
                     goto opt;
                }
                                  
            //exit (after the program processed the input that is converting numbers)
            printf("\n\n\nRun application again (Y/N)?");
            scanf(" %c",&optExit);
            optExitUpper = toupper(optExit);
            //while ((ch = getchar()) != '\n' && ch != EOF);
            getchar();
        } while (optExitUpper == 'Y');
       
        system("pause");
        return 0;
    }

  6. #6
    Registered User
    Join Date
    May 2010
    Posts
    2,560
    Again just get rid of the goto by rearranging your code, do you really need to print "Error"? If not just put your code that asks about running the calculation again in the else.

    Another possibility would be using a switch statement instead of the if/else series, with the error condition in the default clause.

    Jim

  7. #7
    Registered User
    Join Date
    Oct 2012
    Posts
    11
    Em.. If I put the code asking about running the calculation again in the else, then, the users will not come to that questions if they already chose the right number ( 1 or 2 or 3 or 4). My program interface as in the picture.

    Name:  myprog.png
Views: 150
Size:  37.1 KB

  8. #8
    Registered User
    Join Date
    Nov 2010
    Location
    Long Beach, CA
    Posts
    5,309
    Okay, so Jim's suggestion to avoid the 'goto' in post #4 is a little harder since you have an if/else if chain instead of just an if/else. But rather than use 'goto', you should use the 'continue' keyword*. For example:
    Code:
    do {
       get base from user
       switch (base)
           case 1
               binary stuff
               break;  // this breaks out of the switch, not the loop
           ...
           default
               error
               continue  // go back to the top of the loop and start over
       ...
    } while not exit
    Also, your while loop around lines 17-20 could be replaced by an fgets:
    Code:
    fgets(buffer, sizeof(buffer), stdin);
    Note, that will leave the newline ('\n') character in there, but there's a pretty quick way to remove it:
    Code:
    char *p;
    p = strchr(buffer, '\n');  // if there's a newline in the buffer, p will point to it, else it will point to NULL
    if (p != NULL)  // strchr found a new line
        *p = '\0';  // replace the new line with a null terminator
    Generally, it's preferable to use existing functions to do your work, since they're well tested and robust. Your version actually has the potential exceed the bounds of buffer by one byte, since you fill all of buffer with user input, then null terminate the following spot in buffer, which may be off the end of the array if the user provided MAXBUFFERSIZE bytes or more of input. You can also get rid of char_count and userIn if you choose to implement this.

    Last little thing, I don't see much need for optExitUpper. I would remove it and just put the toupper call in the loop condition:
    Code:
    } while (toupper(optExit) != 'Y');
    * The 'continue' keyword takes you back to the top of the loop. There's also the 'break' keyword that takes you out of the loop, to the first statement after it. Many people consider them almost as bad as 'goto', since they jump you around the code somewhat arbitrarily. I think they're fine though, since they're much more controlled in what they do, they produce less "spaghetti code", and they're a bit harder to misuse.

  9. #9
    Left
    Join Date
    Oct 2011
    Posts
    816
    Here you have a loop that reads up to MAXBUFFERSIZE characters from the input into buffer[]:
    Quote Originally Posted by jinjiro View Post
    Code:
            userIn = getchar();
            char_count = 0;
            
            while((userIn != '\n') && (char_count < MAXBUFFERSIZE)) { //avoid user from entering invalid input type
                        buffer[char_count++] = userIn;
                        userIn = getchar();
                    }
    You don't check for end of file, userIn == EOF. This means that if the user presses Ctrl-D (in Linux/Mac, Ctrl-Z in Windows, I think), your buffer will just be filled with garbage ((char)EOF, typically 0xFF.)

    Why don't you do
    Code:
            char  *input_line;
            size_t input_len;
    
            input_line = fgets(buffer, sizeof buffer, stdin);
            if (input_line == NULL) {
                /* No more input! */
                break;
            }
    instead? Getting NULL from fgets() is the same as getting EOF from getchar() .

    Furthermore, you can then remove leading and trailing whitespace and newlines, and find out the input line length by using
    Code:
            input_line += strspn(input_line, "\t\n\v\f\r ");  /* Skip over whitespace */
            input_len = strcspn(input_line, "\n\r"); /* Length until newline or end-of-string */
            input_line[input_len] = '\0';  /* Terminate, in case input_len ended with a newline. */
    Both strcspn() and strspn() are standard C functions. The links tell you more.

    Quote Originally Posted by jinjiro View Post
    Code:
            printf("\n\n\nRun application again (Y/N)?");
            scanf(" %c",&optExit);
            optExitUpper = toupper(optExit);
    Here, you scan for a single character from the standard input. This does not consume the newline that will be in the standard input (as the user must press Enter, there will be a newline in the input). Therefore, on the second run, your earlier read loop will first read the newline, and read nothing.

    To request confirmation, I'd recommend using a loop, such as
    Code:
            do {
                printf("\n\n\nRun application again (Y/N)?");
                fflush(stdout);
    
                /* Read a line of input. */
                input_line = fgets(buffer, sizeof buffer, stdin);
    
                if (input_line == NULL) {
                    /* No more input. Act as if the user said N. */
                    input_line = buffer;
                    input_line[0] = 'N';
                    input_line[1] = '\0';
                } else {
                    /* Skip leading whitespace and newlines. */
                    input_line += strspn(input_line, "\t\n\v\f\r ");
                }
            } while (input_line[0] != 'Y' && input_line[0] != 'y' &&
                     input_line[0] != 'N' && input_line[0] != 'n');
    As to the overall structure:
    Code:
        /* "application" loop */
        do {
    
            do {
                /* TODO: Ask the user for the base */
    
                /* TODO: read line */
    
                /* TODO: parse line to 'base' */
    
            } while (base < 2);
    
            /* No input? break in above do {} while() loop? */
            if (!input_line)
                break;
    
            do {
                /* TODO: ask the user for data to convert */
    
                /* TODO: read line */
    
                /* Empty line ends data loop */
                if (line_len < 1)
                    break;
    
                /* TODO: parse line */
    
                /* TODO: convert */
    
                /* TODO: output results */
    
                /* Repeat forever (unless break above) */
            } while (1);
    
            /* No more input, break in above do {} while () loop? */
            if (!input_line)
                break;
    
            do {
                printf("\n\n\nRun application again (Y/N)?");
                fflush(stdout);
    
                /* Read a line of input. */
                input_line = fgets(buffer, sizeof buffer, stdin);
    
                if (input_line == NULL) {
                    /* No more input. Act as if the user said N. */
                    input_line = buffer;
                    input_line[0] = 'N';
                    input_line[1] = '\0';
                } else {
                    /* Skip leading whitespace and newlines. */
                    input_line += strspn(input_line, "\t\n\v\f\r ");
                }
            } while (input_line[0] != 'Y' && input_line[0] != 'y' &&
                     input_line[0] != 'N' && input_line[0] != 'n');
    
        } while (input_line[0] == 'Y' || input_line[0] == 'y');
    Quote Originally Posted by jinjiro View Post
    Another question is about loop in the code below.
    1. Only ask one question per post.
    2. Don't do string magick on numbers. There are easier ways to do that.
    Code:
    assuming unsigned int or unsigned long number:
     number     % 10U           gives you the first, rightmost digit
    (number / 1U) % 10U         also gives you the first, rightmost digit
    (number / 10U) % 10U        gives you the seconds digit (tens)
    (number / 100U) % 10U       gives you the third digit (hundreds)
    and so on.

    You can even loop over the digits (from right to left) using
    Code:
    unsigned int number = nonnegative number whose digits we are interested in;
    unsigned int divisor = 1U;
    unsigned int position = 0; /* Rightmost */
    unsigned int digit;
    
    do {
        digit = number % 10U;
    
        /* We now have 0 <= digit && digit < 10.
         * TODO: Do something with digit (and possibly position).
        */
    
        /* Divide number by ten, and advance position indicator */
        number /= 10U;
        position++;
    
        /* Loop for as long as we have digits left */
    } while (number);
    Now, it just so happens that decimal system is just base-10. So, if you were to change the number 10 (10U meaning unsigned-integer ten) to any integer larger than one, it would work for that base too. Just remember it goes from right to left; I included the position so it'll start from 0 (rightmost) upwards (left).

  10. #10
    Registered User
    Join Date
    Oct 2012
    Posts
    11
    Thank you jim, anduril and nominal. I've change the code by using the switch statement and it look nicer.

    but I didn't quite understand about the method using fgets. what is the variable used to store user input?

    I'm sorry for asking two questions in a post.
    Mr Nominal, I thought that in hexadecimal, you should used string because you want to display alphabet instead of numbers. That functions is to convert binary to hexadecimal, that's why I used string instead of integers. This is the problems, sometimes it also shows symbols and my way of concatenate the string just add up a lot of string every time I run the application again.

  11. #11
    Registered User
    Join Date
    Nov 2010
    Location
    Long Beach, CA
    Posts
    5,309
    Quote Originally Posted by jinjiro View Post
    but I didn't quite understand about the method using fgets. what is the variable used to store user input?
    It's the same variable you are currently using, buffer. It is just a character array. You don't need to change the declaration of buffer in any way whatsoever. fgets is just a standard C function (like printf, strcpy, etc), that is used to get a line of input from a file, the user, etc. You use stdin as the FILE to read from if you want to read from the keyboard.

  12. #12
    Registered User
    Join Date
    Oct 2012
    Posts
    11

    Solution for first code problem

    This is the code after I fixed it. Thanks guys!

    Code:
    #include <stdio.h>
    #define MAXBUFFERSIZE   80
    int main(){
        
        int base;
        char optExit, userIn, *p;;
        char buffer[MAXBUFFERSIZE]; //sufficient to handle one line
        int char_count; //number of characters read for this line
        
        do{ 
                printf("\n\nBase: ");
                char_count = 0;
                
               fgets(buffer, sizeof(buffer), stdin);
               p = strchr(buffer, '\n');  // if there's a newline in the buffer, p will point to it, else it will point to NULL
               if (p != NULL)  // strchr found a new line
               *p = '\0';  // replace the new line with a null terminato
    
    
               base = atoi(buffer);//convert from char to int
               
                //example
                switch (base){
                       case 1:break;
                            
                       case 2:break;
                            
                       case 3:break;
                            
                       case 4:break;
                            
                       default: printf("Error!");
                                continue;
                }
                               
                //option to exit
                printf("\n\n\nRun application again (Y/N)?");
                scanf(" %c",&optExit);
                getchar();
                
        } while (toupper(optExit) == 'Y');
       
        system("pause");
        return 0;
    }
    And now about converting binary to hexadecimal. Still not working good.

  13. #13
    Registered User
    Join Date
    May 2010
    Posts
    2,560
    And now about converting binary to hexadecimal. Still not working good.
    Have you tried running this function through your debugger? Also returning a pointer to a local variable is incorrect.

    Jim

  14. #14
    Registered User
    Join Date
    Oct 2012
    Posts
    11
    Yup.. at first there's a problem but then I change the arrToStr variable from char to static char, thus the problem is gone..

    hurm.. what is the correct way to return that value?

    and there's also problem with the value inside hexnum[j] when I convert something more than '111111111' (9 digits).

  15. #15
    Registered User
    Join Date
    Oct 2012
    Posts
    11
    I just found out about the first code, it is still wrong.. when the users input an error value in the first time he runs the program, it will stop the program from running.. but it is okay when the error happen after the first valid input.. such as in the image below.

    Name:  att1.png
Views: 95
Size:  23.3 KB Name:  att2.png
Views: 92
Size:  22.6 KB

Page 1 of 2 12 LastLast
Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Possible Logic Error
    By seanksg in forum C Programming
    Replies: 8
    Last Post: 03-14-2011, 06:42 PM
  2. Error in logic
    By Mentallic in forum C Programming
    Replies: 2
    Last Post: 03-28-2010, 08:02 AM
  3. logic Error
    By asmaa in forum C++ Programming
    Replies: 4
    Last Post: 02-13-2009, 09:58 PM
  4. Error in logic
    By mesmer in forum C Programming
    Replies: 2
    Last Post: 10-21-2008, 06:42 AM
  5. Logic Error...
    By Blackroot in forum C++ Programming
    Replies: 6
    Last Post: 01-28-2006, 05:21 AM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21