Why printing arrays of characters gives garbled characters?

Laughingtree 0
Viewed 79

I'm reading characters into an array and try to print them out. The code is given below.

#include <stdio.h>
#include <ctype.h>

#define N 50

int read_message(char a[], int n)
{
    char ch;
    int i = 0;

    printf("Enter a message: ");
    for (i = 0; (ch = getchar()) != EOF && ch != '\n'; i++) {
        if (toupper(ch) >= 'A' && toupper(ch) <= 'Z') {
            a[i] = tolower(ch);
        }
    }

    return i;
}

void print_message(char a[], int n)
{
    int i;

    for (i = 0; i < n; i++)
        printf("%c", a[i]);

    printf("\n");
}

int main()
{
    char message[N];
    int message_len = read_message(message, N);

    print_message(message, message_len);
}

When I enter "He h", it shows "he蘦".

However, if I change the read_message() in the following way, the problem is solved.

int read_message(char a[], int n)
{
    char ch;
    int i = 0;

    printf("Enter a message: ");
    while ((ch = getchar()) != EOF && ch != '\n') {
        if (toupper(ch) >= 'A' && toupper(ch) <= 'Z') {
            a[i] = tolower(ch);
            i++;
        }
    }

    return i;
}

I heard that for and while are equivalent, so I have no idea why this is happening.

while-loop for-loop c
2 Answers

In the first version of read_message() you have the following issues:

  1. int ch as getchar() returns an int (on my system char is unsigned and EOF defined as -1).
  2. Only increment i when you have a valid letter which is in the if body not for each iteration of the loop.
  3. a should be \0-terminate.
  4. a is subject to overflows (i + 1 >= n).
  5. (minor) prefer unsigned types for length (unsigned, size_t etc).
  6. (minor) Use isalpha() just because it's cleaner than the two toupper() calls. Letters are stored in lower case so it's strange that we compare the range in upper case. Other locales may have a different alphabet.
  7. (minor and not fixed to minimize change) replace for with while as it now has neither initialization or increment parts (this would be case even if it was for (i=0; ...) just easier to see this way).
unsigned read_message(char a[], unsigned n) {
    printf("Enter a message: ");
    int ch;
    unsigned i = 0;
    for (; (ch = getchar()) != EOF && ch != '\n' && i + 1 < n;) {
        if (isalpha(ch)) {
            a[i++] = tolower(ch);
        }
    }
    if(n)
        a[i] = '\0';
    return i;
}

I would swap the arguments so you can document the relationship between n and a, and write it like this (as @Lundin wasn't a fan of the above):

#include <ctype.h>
#include <stdin.h>

#define N 50

size_t read_message(size_t n, char a[n]) {
   printf("Enter a message: ");
   size_t i = 0;
   while(i + 1 < n) {
       int ch = getchar();
       if(ch == EOF || ch == '\n') break;
       if(!isalpha(ch)) continue;
       a[i++] = tolower(ch);
   }
   if(n) a[i] = '\0';
   return i;
}

int main() {
    char s[N];
    size_t n = read_message(sizeof(s), s);
    printf("%u, %s\n", n, s);
}

and example execution:

Enter a message: Hello WoRLD 123 how Are You?
19, helloworldhowareyou
Allan Wind0

As a follow up to another answer:

Please for the love of obfuscation never write obscure loops such as
for (; (ch = getchar()) != EOF && ch != '\n' && i + 1 < n;)
Contrary to popular belief, most operators on a single line does not win a price. It might cause you to lose your C programmer job, however...

This code should be de-obfuscated into something readable, using the most canonical form of a for loop (for(int i=0; i<n; i++)) if that is possible:

for(int i=0; i<n; i++)
{
  int ch = getchar();
  if(ch == EOF || ch == '\n')
  { 
    break; 
  }

  if(isalpha(ch))
  {
    a[i] = tolower(ch);
  }
}

Notes:

  • isalpha tests for any character for which isupper or islower is true. Whereas tolower converts any character for which isupper is true. So you may as well use isupper, or even skip the if and just do a[i] = tolower(ch).

  • In case you only want to increase the counter when a certain condition is true, you need a separate counter variable. Do not obfuscate the for loop by moving i++ out of the 3rd clause of the for.

Overall, your code could be simplified a lot and there's no obvious need to keep track of the string length manually in this case. You could simplify the whole thing to this:

#include <stdio.h>
#include <ctype.h>

#define N 50

void read_message (char a[], int n)
{
    int i;

    printf("Enter a message: ");
    for(i=0; i<n; i++)
    {
      int ch = getchar();
      if(ch == EOF || ch == '\n')
      { 
        break; 
      }
      a[i] = tolower(ch);
    }
    a[i] = '\0';
}

int main (void)
{
    char message[N+1];
    read_message(message, N);
    puts(message);
}

Input:

Hello WoRLD 123 how Are You?

Output:

hello world 123 how are you?
Lundin0
Related