Thread: 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.

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

Originally Posted by jimblumberg

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?

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

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;
}

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

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.

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.

Here you have a loop that reads up to MAXBUFFERSIZE characters from the input into buffer[]:

Originally Posted by jinjiro

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.

Originally Posted by jinjiro

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');

Originally Posted by jinjiro

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).

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.

Originally Posted by jinjiro

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.

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.

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

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).

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.