Scan the user input and then work with these letters

Question

The program scans a maximum of 10 chars from user and then offers you 4 options what you can do with that array. Options are:

• 0 = quit the program,
• 1 = prints whole array,
• 2 = count all the small letters,
• 3 = prints only the numbers,
• 4 = change the small letters with the big ones and via versa.

This is my homework for school, so the interface needs to be in Czech. I think that overall it's quite good but I want to improve!!

#include <stdio.h>
int main() {
 int char_count = 10;
 char input_chars[char_count];
 int i = 0;
 int applied_numb = 1;
 int last_index;
 int amount_small_char = 0;
 int ascii_code;
 printf("Zadejte maximalne 10 znaku" );
 do {
 printf(" \n Zadejte %d z %d: ", i, char_count);
 input_chars[i] = getchar();
 getchar();
 i++;
 if(i == char_count) {
 printf("Zadali jste maximalni pocet znaku. \n");
 }
 } while(i < char_count && input_chars[i - 1] != '.' );
 if(input_chars[i - 1] == '.')
 {
 // in case of last element is "." substract 2 from last index(because we used do while cycle (i++))
 last_index = i - 2;
 }
 else
 {
 last_index = i - 1;
 }
 while (applied_numb != 0 && applied_numb <= 4 && applied_numb >= 0)
 {
 printf("Vyberte si co chcete s danymi cisli provest: \n 0) konec programu \n 1) vypis \n 2) pocet malych pismen \n 3) vypsat jen cislice \n 4) zmenit mala/velka pismena\n");
 scanf ("%i", &applied_numb);
 switch (applied_numb)
 {
 case 0:
 printf ("KONEC PROGRAMU, SEE YOU SOON :) ");
 break;
 case 1:
 printf("Vypis vaseho vstupuje: ");
 for(i = 0; i <= last_index; i++)
 {
 printf(" %c",input_chars[i]);
 }
 break;
 case 2:
 for (i = 0; i <= last_index; i++)
 {
 ascii_code = (int)input_chars[i];
 if (ascii_code >= 97 && ascii_code <= 122)
 {
 amount_small_char = amount_small_char + 1;
 }
 }
 printf("Pocet malych pismen ve vasim vstupu je: %d", amount_small_char);
 break;
 case 3:
 printf("Tyto cislice jsou ve vasem vstupu:");
 for (i = 0; i <= last_index; i++)
 {
 ascii_code = (int)input_chars[i];
 if (ascii_code >= 48 && ascii_code <= 57)
 {
 printf(" %c", input_chars[i]);
 }
 }
 break;
 case 4:
 printf("Zmena na mala/velka pismena vypada takto:");
 for (i = 0; i <= last_index; i++)
 {
 ascii_code = (int)input_chars[i];
 if (ascii_code >= 97 && ascii_code <= 122)
 {
 ascii_code = ascii_code - 32;
 printf(" %c", ascii_code);
 }
 else if(ascii_code >= 65 && ascii_code <= 90)
 {
 ascii_code = ascii_code + 32;
 printf(" %c", ascii_code);
 }
 else if((ascii_code <= 64 && ascii_code >= 0) ||
 (ascii_code >= 91 && ascii_code <= 96) ||
 (ascii_code >= 123))
 {
 printf(" %c", input_chars[i]);
 }
 }
 break;
 }
 printf("\n \n ------------------------------------- \n \n ");
 }
 return 0;
}

Answer 1

You just wrote this program and it's perfectly clear in your mind, imagine to come back after 1 year: you will need to re-read everything to understand what it is doing because everything is in one big function (which is also hard to test, something you will definitely love to have when writing production code).

First of all let's split the logic into smaller functions, do it in small steps:

#define MAX_INPUT_LENGTH 10
int main()
{
 char input[MAX_INPUT_LENGTH + 1] = {0};
 if (!read_input_text(input, MAX_INPUT_LENGTH))
 return EXIT_FAILURE;
 while (1)
 {
 print_menu();
 switch (read_menu_option())
 {
 case 0:
 puts("KONEC PROGRAMU, SEE YOU SOON :) ");
 return EXIT_SUCCESS;
 case 1:
 print_input(input);
 break;
 case 2:
 print_count_lowercase_letters(input);
 break;
 case 3:
 print_digits(input);
 break;
 case 4:
 print_input_toggled(input);
 break;
 default:
 puts("Invalid option.");
 }
 }
 return EXIT_SUCCESS;
}

You can further split this code and menu may be well handled with a dictionary of functions to call (it will be easier to extend) but I think you understand the point. Few things to note:

• I initialized input to zero, now you do not need to last_index because you simply can use strlen(). Also to print the string is as simple as puts(input) because you have a valid null terminated C string (note the extra array item added to hold the string terminator).
• Variable-length arrays are a subtle thing, you may avoid them unless they're absolutely required.
• I'm using EXIT_SUCCESS instead of 0, it's a small thing but I think it helps to make the intent clear (and it's easier to search...)
• I'm using puts() instead of printf() when I simply need to print a string to stdout: it's faster, easier and better communicate the intent (compiler may also detect what you're doing and use puts() for you, BTW).

Now let's examine the core logic. As you can already imagine the input function might be simplified:

bool read_input_text(char* input, size_t maximumLength)
{
 for (size_t i=0; i < maximumnLength; ++i)
 {
 int c = getchar();
 if (c == EOF)
 return false;
 if (c == '.')
 return true;
 input[i] = (char)c;
 }
 return true;
}

Few notes:

• I'm using a for loop to read needed characters, I think it's shorter and easier to understand than the equivalent while.
• I check for EOF, user may press CTRL+D/CTRL+Z or stdin may be redirected and input file may be too short. As Chux noted you may consider EOF when string is still empty as a valid input (returning true).
• When user type . I do not write it to input and because it's already zeroed then I simply can exit.
• I'm using bool and other macros defined in stdbool.h.
• I'm not allocating memory in this function, you may also change it to be char* read_input_text() but it's less error-prone (in bigger programs) to have a clear ownership for dynamically allocated memory.
• read_input_text() is assuming that input is already zeroed. In true production code this should not be the case (less function interface requirements mean less bugs...) You may try to change it to append a trailing 0 where appropriate (tnx Chux, again, to point this out.)

How to count for lowercase letters? You're doing that manually but it's little bit hard to read because you're using magic numbers, let's try to write a small function:

int count_lowercase_characters(char* text)
{
 int count = 0;
 for (size_t i=0; i < strlen(text); ++i)
 {
 if (text[i] >= 'a' && text[i] <= 'z')
 ++count;
 }
 return count;
}

Using 'a' and 'z' instead of their ASCII values makes code easier to understand (I personally do not remember ASCII codes...) and less error-prone. But, hey...isn't it a very common task? There is a standard function for that, let's use it:

int count_lowercase_characters(const char* text)
{
 int count = 0;
 for (size_t i=0; i < strlen(text); ++i)
 {
 if (islower((unsigned char)text[i])
 ++count;
 }
 return count;
}

Note:

• I'm directly using strlen() and loop variable is size_t. Note that it's not mandatory for size_t to be equivalent to int!
• I'm using int even if unsigned (or even better size_t) might be more appropriate, don't go into academic discussions about this...
• I declare text as const char*, I'm not going to modify the pointed value. Later you may want to declare it as const char* const but no need to go that far, now (also consider to declare it as char const *, it may be easier to pick when beginning).
• I'm casting text[i] to unsigned char, if your compiler is aliasing char to unsigned char then you do not need it. See also Do I need to cast to unsigned char before calling toupper?

To write print_count_lowercase_letters() should now be trivial. As you can imagine there is also a similar function isdigit() to determine if a char is a digit (0..9) to use in print_digits().

The last function we have to go through is the one to toggle the case:

if (islower(text[i]))
 text[i] = toupper(text[i]);
else if (isupper(text[i]))
 text[i] = tolower(text[i]);

In this case I'm toggling the original string but you may simply write it to stdout. Note that you're using printf() to print a single character, there is an easier way:

if (islower((unsigned char)text[i]))
 putchar(toupper(text[i]));
else if (isupper((unsigned char)text[i]))
 putchar(tolower(text[i]));
else
 putchar(text[i]);

Notes:

• I'm using putchar() to print a single character to stdout. Easier and faster.
• I'm using islower() and isupper() to determine the case of the character. If you can't use these library functions then write your own function to reuse.
• I'm using tolower() and toupper() to perform case conversion. Again if you can't use them then write your own, you will gain in legibility.
• Last else has been greatly simplified, string is user input then it already contains printable characters, everything else which is not a letter should simply go untouched.
• I didn't write it but the loop is similar to the previous function for (size_t i=0; i < strlen(text); ++i). See very last paragraph for more info.

---

In this examples I didn't consider any optimization, you will soon learn that strlen(text) as condition in a for loop is relatively terrible from a performance point of view (because outside very simple cases compiler isn't able to determine that it can be optimized and evaluated just once). The same loop may then be rewritten (few solutions ordered for complexity+speed).

To evaluate strlen(text) just once:

for (size_t i=0, n = strlen(text); i < n; ++i)

To manually stop when encountering string terminator:

for (size_t i=0; text[i] != 0; ++i)

To fully use pointers arithmetic:

for (char *p = text; *p; ++p)

• user52292: +1, but please for (size_t i=0, n = strlen(text); i < n; i++) or even better: for(char *p = text; *p; p++)
• Adriano Repetti: @firda OP is still studying, I left out any optimization in favor of clarity but yes, I agree it is worth to be mentioned (at least for future readers!)
• user52292: I agree that performance optimization is not on the table of this review, but I still think that you, as a reviewer, should not offer such code. I actually like your version with text[i] the most, apropriate for OP's level of knowledge (no pointers) and you already zeroed the input array to use strlen because it searches for terminator. So maybe: for (size_t i=0; text[i] != 0; i++) would be most educational.
• chux: @AdrianoRepetti Interesting thought about using a return type like std::count() that returns some "signed integral type". So code could still use size_t count ; to perform that count (as that will never overflow, unlike int). Then perform a range test if desired just prior to converting and returning that "signed integral type". e.g. if (count > INT_MAX) TBD_Handler(); else return (int) count;
• chux: @firda islower(-1) does not convert the -1 to an unsigned char. With the common case that EOF == -1, islower(-1) returns 0 because EOF is not a lower case letter. islower(int ch) is defined for typically 257 different inputs. There is no conversion to unsigned char of the int argument. Do I need to cast to unsigned char before calling toupper? and argument of isupper/islower etc. needs to be converted to unsigned char may be useful.