Strange behaviour in C programming

Question

The program bellow is supposed to do two things. First is to reverse a string and the second is to find in that string the most and the least seen character. It works almost fine. The results are correct but on finishing it does not return 0 and second if I delete the line 16 puts("\n") the program does not work. That specific line obviously does nothing, but without it, it does not work. I tried to replace it with fflush(stdin) but it did not work. I do not get it. What is wrong?

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <stdbool.h>

void chars(char *s, char *most_seen, char *less_seen)
{
    int letters[26] = {};

    for (int i = 0; i < strlen(s); i++)
    {
        char c = *(s + i);
        letters[(int)c - 97]++;
    }

    puts("\n");
    int maxLetter = 0;
    int minLetter = 0;
    int posMax = 0, posMin = 0;

    for (int i = 0; i < strlen(s); i++)
    {
        if (letters[i] > maxLetter)
        {
            maxLetter = letters[i];
            posMax = i;
        }
        if (letters[i] < minLetter)
        {
            if (letters[i] == 0) continue;
            minLetter = letters[i];
            posMin = i;
        }
    }
    *most_seen = posMax + 97;
    *less_seen = posMin + 97;
}

char *reverse(char *s)
{
    char *reversed = (char *)malloc(strlen(s) * sizeof(char));
    fflush(stdin);
    int i = 0, size = strlen(s) - 1;
    for (; i < strlen(s); i++)
        reversed[i] = s[size--];
    reversed[i] = '\0';

    return reversed;
}

int main()
{
    char *s = malloc(1000 * sizeof(char));
    char *s2;

    char mostSeen, lessSeen;

    gets(s);
    chars(s, &mostSeen, &lessSeen);
    s2 = reverse(s);
    printf("%s\n", s);
    printf("%s\n", s2);
    printf("%c %c\n", mostSeen, lessSeen);

    free(s);
    free(s2);

    return 0;
}

Answer 1

There are multiple problems in your code:

• [major] you do not allocate enough space for the reversed string: you must allocate one extra byte for the null terminator.
• [major] the initializer int letters[26] = {}; is invalid in C. You probably compile the code as C++, where this syntax is allowed, but beware that C++ is a different language, not a strict superset of C.
• [major] you should test if the character from the string is between 'a' and 'z' before incrementing letters[(int)c - 97]. As coded, you have undefined behavior if the string contains any character outside the range 97..122.
• [major] the second loop in chars() should run from i = 0 to i < 26.
• [major] you initialize minLetter to 0, so the test if (letters[i] < minLetter) will always evaluate to false. You should instead initialize minLetter to the length of the string plus one.
• [minor] you compute the length of the string too many times. You should store it in a local variable.
• [style] the syntax *(s+i) is valid, but it is much more readable to write str[i].
• [style] you should use 'a' instead of 97, both for portability and readability.
• [major] the function gets() has been removed from the C Standard because there is no way to tell the C library the size of the array, so any sufficiently long input line will cause undefined behavior (buffer overflow). Use fgets() instead and remove the trailing newline.
• [minor] there is no need to allocate s: make it a local char array (automatic storage).
• [minor] there is no need to cast the return value of malloc() in C, but it is mandatory in C++.
• [minor] the size of char is 1 by definition in C.
• [major] you do not check for memory allocation failure.
• [minor] if the string is empty, you return 'a' as the most and least frequent letter, which is incorrect.

Here is a modified version:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

size_t chars(const char *s, char *most_seen, char *least_seen) {
    /* only count lowercase letters */
    size_t counts['z' - 'a' + 1] = { 0 };
    size_t len = strlen(s);

    for (size_t i = 0; i < len; i++) {
        int c = s[i];
        if (c >= 'a' && c <= 'z')
            counts[c - 'a']++;
    }

    size_t maxCount = 0, minCount = len + 1;
    char maxLetter = 0, minLetter = 0;

    for (int c = 'a'; c <= 'z'; c++) {
        size_t count = counts[i - 'a'];
        if (count != 0) {
            if (count > maxCount) {
                maxCount = count;
                maxLetter = c;
            }
            if (count < minCount) {
                minCount = count;
                minLetter = c;
            }
        }
    }
    *most_seen = maxLetter;
    *least_seen = minLetter;
    return len;
}

char *reverse(const char *s) {
    size_t len = strlen(s);
    char *reversed = malloc(len + 1);
    if (reversed != NULL) {
        for (size_t i = 0; i < len; i++) {
            reversed[i] = s[len - i - 1];
        }
        reversed[len] = '\0';
    }
    return reversed;
}

int main() {
    char s[1000];
    char *s2;
    char mostSeen, leastSeen;

    if (fgets(s, sizeof s, stdin)) {
        /* strip the trailing newline, if any */
        size_t len = strlen(s);
        if (len > 0 && s[len - 1] == '\n') {
            s[--len] = '\0';
        }
        printf("%s\n", s);
        s2 = reverse(s);
        if (s2 == NULL) {
            printf("memory allocation failed!\n");
        } else {
            printf("%s\n", s2);
            free(s2);
        }
        if (chars(s, &mostSeen, &leastSeen) == 0) {
            printf("string is empty.\n");
        } else {
            printf("%c %c\n", mostSeen, leastSeen);
        }
    }
    return 0;
}