Inputting two "Enter a letter"

Good evening. I'm writing a program to squeeze a string removing every character the user wants. I used fgetc(stdin) for input but I'm getting that buffer problem:

Code:

Enter the word you want: thames
Enter the number of letters of the alphabet you want (1-26): 4
Enter a letter: Enter a letter: t
Enter a letter: Enter a letter: h

t
h

P.S.: I also tried to scanf the string and use getchar with c.

Code:

#include <stdio.h> 
#include <stdlib.h> /* for malloc() and free() */
#include <ctype.h>

#ifndef MAXLINE 
  #define MAXLINE 100 
#endif

#ifndef ALPHA
  #define ALPHA 26 
#endif  

/* the function removes t characters chosen by the user from the s string */
void squeeze(char* s, char* t) 
{ 
    
}     

int main(void) 
{ 
  char* s;
  char* t;
  int c;
  int n;
  int i = 0;
  
  s = malloc(MAXLINE * sizeof(char*));
  t = malloc(ALPHA * sizeof(char*));
  
  printf("Enter the word you want: ");
  fgets(s, MAXLINE, stdin);
  
  do { 
    printf("Enter the number of letters of the alphabet you want (1-26): ");
    scanf("%d", &n);
  
  } while(n < 1 || n > 26);
  
  while(i < n) 
  {  
     printf("Enter a letter: ");
     c = fgetc(stdin);
     
     if( (isalpha(c) == EOF))
     { 
       printf("The input character is invalid");
       exit(0);    
     }
     
     t[i++] = c;
  }       
         
  printf("%s\n", t);
  
  free(s);
  free(t);
  return 0;
}

This is a very common problem when mixing line input (fgets), formatted input (scanf) and character input (getchar/fgetc). After you input 4, you press enter, which puts a '\n' (new line) in the input buffer. The scanf ("%d", ...) doesn't process that newline, so it is left in the buffer. Then, when you read a character (whether it be fgetc, getchar, scanf("%c")), you read the newline char and try to process it. Try flushing the input buffer. Just do NOT use fflush(stdin)!

This can get a bit crazy. I mean I've been inputting fgets and outputting "you entered: ... " for almost everything.

everything went fine (not counting the need of malloc the new string strnum to get a representation of string of a number because of the buffer) until I had to enter the chars. Fgetc is not working properly and I'm getting this kind of output:

Code:

 
Enter the word you want: thames
You entered thames
Enter the number of letters of the alphabet you want (1-26): 4
You entered: 4Enter a letter: c
cYou entered: cEnter a letter: 
You entered: 
Enter a letter: a
aYou entered: aEnter a letter: 
You entered: 
c
a

an "You entered: " after "Enter a letter:"

Code:

 
#include <stdio.h> 
#include <stdlib.h> /* for malloc() and free() */
#include <ctype.h>

#ifndef MAXLINE 
  #define MAXLINE 100 
#endif

#ifndef ALPHA
  #define ALPHA 26 
#endif  

/* the function removes t characters chosen by the user from the s string */
void squeeze(char* s, char* t) 
{ 
    
}     

int main(void) 
{ 
  char* s;
  char* t;
  int c;
  int n;
  char* strnum;
  int i = 0;
  
  s = malloc(MAXLINE * sizeof(char*));
  t = malloc(ALPHA * sizeof(char*));
  strnum = malloc(sizeof(char*));
  
  printf("Enter the word you want: ");
  
  if(fgets(s, MAXLINE, stdin))
  { 
    printf("You entered %s", s);   
  }       
  
  do { 
    printf("Enter the number of letters of the alphabet you want (1-26): ");
    
    if( (fgets(strnum, sizeof(char*), stdin)) ) 
     n = atoi(strnum);      
         
   } while(n < 1 || n > 26);
  
  printf("You entered: %d", n); 
  
  while(i < n) 
  {  
     printf("Enter a letter: ");
      if( (c = fgetc(stdin)) )
        printf("You entered: %c", fputc(c, stdout));
     
     if( (isalpha(c) == EOF))
     { 
       printf("The input character is invalid");
       exit(0);    
     }
     
     t[i++] = c;
  }       
         
  printf("%s\n", t);
  
  free(s);
  free(t);
  return 0;
}

I don't really know what you mean by "need of malloc". You do not need malloc for this program, and you're using it incorrectly.

Code:

s = malloc(MAXLINE * sizeof(char*));
t = malloc(ALPHA * sizeof(char*));
strnum = malloc(sizeof(char*));

Those should be sizeof(char), not sizeof(char*). You want to allocate an array of chars, not char pointers (an array of char pointer would be used to keep an array of words/strings). Also, strnum should be something like (MAXLINE * sizeof(char)), otherwise there isn't enough space if the user enters larger numbers. Lastly you forget to call free(strnum).

But all that is moot, there is no need for malloc, so you should be using regular char arrays:

Code:

char s[MAXLINE];
char t[ALPHA];
char strnum[MAXLINE];
...
fgets(strnum, sizeof(strnum), stdin);
// similar for s and t

Note I use sizeof(strnum) instead of MAXLINE. That way, if you decide to change strnum to be of some other size later (say, of size ALPHA), you don't have to change your call to fgets, it will still be correct.

Read this: FAQ-Draft: Why scanf("%c", &c) doesn't work as expected.

You're getting that kind of output because after you hit 'c', you hit enter. fgetc reads that enter as a regular char, a '\n' (newline) to be specific. That is how it is supposed to work, so you have to work around that. I gave you a link in my first post to flushing the input buffer. Alternatively, you could simply use fgets to read it into one of your char arrays and only look at the first character of the array. What you can not do is use fgetc and expect it to magically ignore the newline.

I fixed the problem using a getc(stdin) for nothing:

Code:

 
for(i = 0; i < n; i++)
  {  
     printf("Enter a letter: ");
      if( (c = fgetc(stdin)) != EOF) 
        t[i] = c;
     getc(stdin);
     printf("You entered: %c\n", c);
   }

however, I won't get any char if I write:

Code:

if( (c = isalpha(fgetc(stdin)) ) != 0)
  // the same code

also, why does c print 1024?

Code:

Starting program: /home/thames/C/prosqueeze 
Enter the word you want: thames
You entered thames
Enter the number of letters of the alphabet you want (1-26): 4
You entered: 4
Enter a letter: t

Breakpoint 1, main () at prosqueeze.c:54
54         getc(stdin);
(gdb) print c
$1 = 1024
(gdb) cont
Continuing.
Enter a letter: h

Breakpoint 1, main () at prosqueeze.c:54
54         getc(stdin);
(gdb) print t
$2 = 0x602080 ""
(gdb) print c
$3 = 1024

Since you didn't provide the new version of your code, I must assume you only made the changes you mention in the above post.

Originally Posted by thames

I fixed the problem using a getc(stdin) for nothing:

Code:

 
for(i = 0; i < n; i++)
  {  
     printf("Enter a letter: ");
      if( (c = fgetc(stdin)) != EOF) 
        t[i] = c;
     getc(stdin);
     printf("You entered: %c\n", c);
   }

This is still not a fix. All it amounts to is a broken implementation of fgets, which you should probably be using. You do no bounds checking, so I can easily go past the end of the t array. Also, the getc(stdin) discards every other character. If I input "abcd", the array t will be filled with 'a' and 'c', and you will still have the dangling new line screwing up further input.

Also, since you didn't say anything, I am assuming you did not fix your malloc issues. Your calls to malloc are still wrong, char * is the wrong type. I still recommend a regular char array (there is no need for malloc here), but if you insist on malloc'ing data, at least use the right type for sizeof. Try

Code:

#define STRNUM_MAX    10
char s[MAXLINE];
char t[ALPHA];
char strnum[STRNUM_MAX];  // or something similar

But, if you insist on using malloc when it's not the best solution, at least use it correctly:

Code:

// note, size of a plain char, not a char *
s = malloc(MAXLINE * sizeof(char));
t = malloc(ALPHA * sizeof(char));
strnum = malloc(STRNUM_MAX * sizeof(char));

however, I won't get any char if I write:

Code:

if( (c = isalpha(fgetc(stdin)) ) != 0)
  // the same code

Of course not. That if statement doesn't assign the character read into the variable c. It passes that character to isalpha, which uses it to return a simple true/false value. You should only check that in a boolean manner, don't compare to EOF (which is not guaranteed to be any one specific value). You want something like:

Code:

c = fgetc(stdin);
if (!isalpha(c))
    // same code

also, why does c print 1024?

Code:

Starting program: /home/thames/C/prosqueeze 
Enter the word you want: thames
You entered thames
Enter the number of letters of the alphabet you want (1-26): 4
You entered: 4
Enter a letter: t

Breakpoint 1, main () at prosqueeze.c:54
54         getc(stdin);
(gdb) print c
$1 = 1024
(gdb) cont
Continuing.
Enter a letter: h

Breakpoint 1, main () at prosqueeze.c:54
54         getc(stdin);
(gdb) print t
$2 = 0x602080 ""
(gdb) print c
$3 = 1024

I can't replicate that behavior, but I may be using different code than you. You need to post your current code if you want more help.

I'm using malloc for training. Thank you for your concern, it's very kind. when I get stubborn it's for training. Sorry for not giving further information about modification.

The current code is:

Code:

#include <stdio.h> 
#include <stdlib.h> /* for malloc() and free() */

#ifndef MAXLINE 
  #define MAXLINE 100 
#endif

#ifndef ALPHA
  #define ALPHA 26 
#endif  

/* the function removes t characters chosen by the user from the s string */
void squeeze(char* s, char* t) 
{ 
    
}     

int main(void) 
{ 
  char* s;
  char* t;
  char* strnum;
  int n;
  int i = 0;
  int c;
  
  s = malloc(MAXLINE * sizeof(char));
  t = malloc(ALPHA * sizeof(char));
  strnum = malloc(sizeof(char));
  
  printf("Enter the word you want: ");
  
  if(fgets(s, sizeof(s), stdin))
  { 
    printf("You entered %s", s);   
  }       
  
  do { 
    printf("Enter the number of letters of the alphabet you want (1-26): ");
    
    if(fgets(strnum, sizeof(strnum), stdin)) 
     n = atoi(strnum);      
         
   } while(n < 1 || n > 26);
  
  printf("You entered: %d\n", n); 
  
  for(i = 0; i < n; i++)
  {  
     printf("Enter a letter: ");
      if( (c = fgetc(stdin)) != EOF) 
        t[i] = c;
     getc(stdin);
     printf("You entered: %c\n", c);
   }       
         
  for(i = 0; i < n; i++) 
  { 
    printf("%c\n", t[i]);  
  }       
  
  free(s);
  free(t);
  free(strnum);
  
  return 0;
}

It passes that character to isalpha, which uses it to return a simple true/false value. You should only check that in a boolean manner, don't compare to EOF

I thought it was the integer value of c (but, to be honest, I forgot that "is" relates to boolean).

Originally Posted by thames

how would I know this? I mean, man isalpha says the return type of isalpha is an integer. I thought it was the value of c (still, to be honest, I forgot that "is" relates to boolean).

Don't just read the prototype. Try reading the whole man page (or at least all the relevant parts).

Originally Posted by man isalpha

RETURN VALUE
The values returned are nonzero if the character c falls into the tested class, and a zero
value if not.

The man pages for any C function will have a "RETURN VALUE" section, describing the value returned.

I see one problem right off the bat:

Code:

strnum = malloc(sizeof(char));

I told you you need something like STRNUM_MAX in there. Your way, you're allocating space for 1 char, which is not enough for the string you read in with fgets later. There is room for one char, but when fgets adds the newline and the null terminator, you have a buffer overflow. Hello undefined behavior! Given that anything can happen in undefined behavior, this alone could be the root of your problem.

Next problem:

Code:

sizeof(s)
sizeof(strnum)

Those don't give you the values you think they do. Read this: Question 7.28 (that site has lots of good info, you should read it from time to time). The sizeof only works with arrays. For malloc, you basically need use the same expression you passed to malloc:

Code:

s = malloc(MAXLINE);
strnum = malloc(STRNUM_MAX);
...
fgets(s, MAXLINE, stdin)
fgets(strnum, STRNUM_MAX, stdin)

Notice I left off the * sizeof(char) part. That is technically not needed, since the standard guarantees sizeof(char) will always be 1, thus this way is a bit shorter.

After fixing all that, I can not replicate the "1024" issue you were having, and a quick run of the code with electric fence (link) and valgrind (link) seem to show it running cleanly. Note, both of those are invaluable tools for debugging memory issues. I strongly suggest you get used to working with them, they're both quite easy to use in their simplest forms. They have very powerful options, which you probably wont need to worry about 99% of the time.

I decided to use a different approach: get a string of letters (with fgets) separated by commas then delete the commas that were input to get a clean letter array. However, my logic is not correct. I debugged with gdb:

Code:

 
Please enter 4 letters separated by commas: a,b,c,d

Breakpoint 1, main () at prosqueeze.c:80
80           if(cond == 1 && len == n)
(gdb) print cond
$1 = 0
(gdb) print len
$2 = 8

Code:

#include <stdio.h> 
#include <stdlib.h> /* for malloc() and free() */
#include <ctype.h>
#include <string.h>

#ifndef MAXLINE 
  #define MAXLINE 100 
#endif

#ifndef ALPHA
  #define ALPHA 26 
#endif  

#ifndef STRNUM_MAX
  #define STRNUM_MAX 10
#endif  

void squeeze(char*, char*);
void noCommas(char*);
int isStrAlpha(char*);

/* the function removes t characters chosen by the user from the s string */
void squeeze(char* s, char* t) 
{ 
    
}     

/* noCommas: delete all commas from s */
void noCommas(char* s)
{ 
   int i, j;
   for(i = 0, j = 0; s[i] != '\0'; i++) 
    if(s[i] != ',') 
     s[j++] = s[i];
 }     
 
 /* isStrAlpha: verify if the entire string is alphabetic */
 int isStrAlpha(char* s)
 { 
    int i; 
    for(i = 0; s[i] != '\0'; i++) 
      if(!isalpha(s[i]))
         return 0;
    return 1;      
  }      

int main(void) 
{ 
  char* s;
  char* t;
  char* strnum;
  int n;
  int i = 0;
  int len = 0;
  int cond = 0;
  
  s = malloc(MAXLINE * sizeof(char));
  t = malloc(ALPHA * sizeof(char));
  strnum = malloc(STRNUM_MAX * sizeof(char));
  
  printf("Enter the word you want: ");
  fgets(s, MAXLINE, stdin);
  printf("You entered: %s", s);
        
  do { 
    printf("Enter the number of letters of the alphabet you want (1-26): ");
    
    if(fgets(strnum, STRNUM_MAX, stdin)) 
      n = atoi(strnum);      
         
   } while(n < 1 || n > 26);
  
  printf("You entered %s", strnum); 
  
  printf("Please enter %d letters separated by commas: ", n);
    if(fgets(t,ALPHA,stdin))       
    {  
       noCommas(t);
       cond = isStrAlpha(t);
       len = strlen(t);
       
       if(cond == 1 && len == n)
       { 
         squeeze(s, t);   
       }            
       else 
       {   
         printf("You entered an invalid character.\n");
         exit(0);
       }
    }     
                
  for(i = 0; i < n; i++) 
  { 
    printf("%c ", t[i]);  
  }       
  
  free(s);
  free(t);
  free(strnum);
  
  return 0;
}

Now you've introduced a different buffer overflow. If the user says they want to enter 26 letters, and they put commas in between each one, fgets wont read them all because 26 letters + 25 commas is 51 characters, and t is only ALPHA (or 26) chars long. Actually, it only takes 14 letters with commas between to cause this overflow.

I think the problem with isStrAlpha being false is that you are not moving the null terminator down when you call noCommas, i.e. when you input "a,b,c,d", it becomes "abcdc,d" (note the leftovers from the original string) instead of "abcd". You can fix that by explicitly null terminating s in your noCommas function if you want.

If I were writing this, here is the approach I would use: Go back to reading one char at a time, but read each one in to a temporary buffer using fgets. That takes care of the dangling newline problem (fgets reads the newline and puts it in your buffer). Get rid of the newline, then check if the buffer has length 1 and the only char in it is an alpha char. If so, put it in t, otherwise, error, try again:

Code:

int i = 0;
int done = 0;
char temp[MAXLINE];
char *p;

do {
    fgets(temp, sizeof(temp), stdin);
    p = strchr(temp, '\n');  // look for the newline read by fgets
    if (p) {
        *p = '\0';  // found it, now get rid of it
    }
    if (strlen(temp) == 1 && isalpha(temp[0])) {  // only 1 char and it's an alpha
        t[i++] = temp[0];  // store it in t
    }
    else {
        printf("You entered an invalid character.\n");
        // dont exit, let them retry incase of a typo
    }
} while (i < n);  // this stops when you got n valid alpha chars from the user
t[i] = '\0';  // null terminate t -- this is only necessary if you actually use t as a string instead of a char array

That code is untested, but it should give you an idea of how it should work, and you should be able to work out any minor bugs.

EDIT: I just realized, you should malloc(ALPHA + 1) chars for t, so you have room for 26 letters and a null terminator (27 in total).

great! now I think it's done. Many thanks.

Code:

#include <stdio.h> 
#include <stdlib.h> /* for malloc() and free() */
#include <ctype.h>
#include <string.h>

#ifndef MAXLINE 
  #define MAXLINE 100 
#endif

#ifndef ALPHA
  #define ALPHA 26 
#endif  

void squeeze(char*, char*);

/* the function removes t characters chosen by the user from the s string */
void squeeze(char* s, char* t) 
{ 
   int i, j, k; 
   for(i = 0; t[i] != '\0'; i++) 
    for(j = 0, k = 0; s[j] != '\0'; j++) 
    { 
      if(s[j] != t[i])
      { 
        s[k++] = s[j];      
      }        
    }
    s[k] = '\0';         
}     

int main(void) 
{ 
  char* s;
  char* temp;
  char* t;
  char* strnum;
  char* p;
  int n;
  int i = 0;
  
  s = malloc(MAXLINE * sizeof(char));
  temp = malloc(MAXLINE * sizeof(char));
  t = malloc((ALPHA + 1) * sizeof(char));
  strnum = malloc(MAXLINE * sizeof(char));
  
  printf("Enter the word you want: ");
  fgets(s, MAXLINE, stdin);
  printf("You entered: %s", s);
        
  do { 
    printf("Enter the number of letters of the alphabet you want (1-26): ");
    
    if(fgets(strnum, MAXLINE, stdin)) 
      n = atoi(strnum);      
         
   } while(n < 1 || n > 26);
  
  printf("You entered %s", strnum); 
  
  do { 
    printf("Enter a letter: ");
    fgets(temp, MAXLINE, stdin);       
    p = strchr(temp, '\n');
    if(p) 
    { 
       *p = '\0';  
    }
    if(strlen(temp) == 1 && isalpha(temp[0]))
    { 
       t[i++] = temp[0];    
    }
    else { 
      printf("You entered an invalid character.\n");    
    }                        
  } while(i < n);
  t[i] = '\0';    
                
  squeeze(s,t);
  
  printf("The squeezed word is %s", s);
  
  free(s);
  free(t);
  free(strnum);
  
  return 0;
}

Code:

temp = malloc(MAXLINE * sizeof(char));

Where do you free temp? I already told you to look into tools like valgrind (see post #8, near the bottom).

Sorry, I will do this. I just didn't have time yet.

I'm trying to use valgrind:

Code:

valgrind --tool=memcheck --leak-check=yes prosqueeze

error:

Code:

valgrind: prosqueeze: command not found

prosqueeze is the name of the executable ./prosqueeze

Originally Posted by thames

prosqueeze is the name of the executable ./prosqueeze

Hmm...try

Code:

valgrind --tool=memcheck --leak-check=yes ./prosqueeze

with the dot-slash, it may be a path issue.