Checking for palindromes using dynamic memory allocation

This is a homework assignment, but it's already done and it's not for a grade; it's meant as a refresher for the rest of the course. I've never done anything with dynamic memory allocation in C before, so I'm assuming I've done at least something wrong.

The goal is to write a program that takes a string from a user and checks to see if it's a palindrome. All non-letter characters are ignored, and dynamic memory allocation is required.

It works fine and I'm pretty happy with it, but I have no way of knowing if I've done things the "right" or "wrong" way. So, could anyone look over it really quickly and point out all the stupid things I've done?

#include 
#include 
#include 

int main(void)
{
    //str holds raw input
    //str_san holds sanitized input
    char *str = (char *)malloc(50), *str_san;
    //i and k are the index counters
    //len is the length of the sanatized string
    int i = 0, k = 0, len;

    //get a string from the user
    printf("Enter a message: \n");
    //store it in str
    scanf("%s", str);

    //make str_san as small as possible
    str_san = (char*)malloc(strlen(str) + 1);

    //copy only letters into the sanitized string
    while (str[i] != '\0') {
        if (str[i] >= 'A' && str[i] = 'a' && str[i] <= 'z') {
            str_san[k] = str[i];
            k++;
        }

        i++;
    }

    //str is no longer needed
    free(str);

    //terminate sanitized string
    str_san[k] = '\0';

    //set len to the length of str_san
    len = strlen(str_san);

    //check for palindrominess
    for (i = 0; i < len; i++) {
        //if the first letter does not equal the last letter then it's not a palindrome
        if (str_san[i] != str_san[len - i - 1]) {
            printf("Not a palindrome");
            return 0;
        }
    }

    //it is now safe to free str_san
    free(str_san);

    //if we've made it this far, it's a palindrome
    printf("Palindrome");

    return 0;
}

You should consider making this more modular (more functions). It's not good to implement an entire program in main() as it could hurt readability. While the separate comments are helpful, it's not a substitute for utilizing functions.

Essentially, you should just do the user-input and the function-calling in main(). In general, try to have main() do as less as possible. For instance, the string-copying and palindrome check should be done in separate functions, and main() should pass the strings to them at the calls.

void copyIntoString(char* str_san) {
    // ...
}

void checkForPalindromes(char* str_san) {
    // ...
}

The deallocation should still be done in main() since the allocation was done in there. Also, the last output can be put into checkForPalindromes() since it's part of that functionality.

I also have some additional notes:

-
If you need a separate variable like len, consider giving it a more accurate name so that it's known which length is being stored. You have two strings, so the reader could easily forget which string to which it belongs. Adding additional strings would then make it worse.

-
Instead of using printf() for unformatted outputs:

printf("Enter a message: \n");

use puts():

puts("Enter a message: ");

It also gives you a newline, so you can leave out yours (unless you want two).

-
It's clearer to have each declaration or initialization done on a separate line, so instead of this:

int i = 0, k = 0, len;

you should have this:

int i = 0;
int k = 0;
int len;

However, as the first two are loop counter variables, they should be placed right in front of the respective for loops (or in the for loops if you have C99 or above). This will help with keeping close scope, which is good for readability and maintenance.

-
This doesn't need to be a while loop:

while (str[i] != '\0') {
    if (str[i] >= 'A' && str[i] = 'a' && str[i] <= 'z') {
        str_san[k] = str[i];
        k++;
    }

    i++;
}

Since i is being incremented each time, it can just be a for loop:

for (int i = 0; str[i] != '\0'; i++) {
    if (str[i] >= 'A' && str[i] = 'a' && str[i] <= 'z') {
        str_san[k] = str[i];
        k++;
    }
}

In addition, you can use isupper() instead of your own check:

if (isupper(str[i])) {

You can also use tolower() to change a character to lowercase:

str_san[k] = tolower(str[i]);

The second if can then use islower():

if (islower(str[i])) {

Lastly, those two if can be consolidated into one. This can be done by calling tolower() whenever any letter is encountered (the character will remain intact if it cannot be made lowercase). Since all these operations only apply to letters, each character can first be checked with isalpha().

All of these function calls may add slight overhead, but if you don't care too much about efficiency or will mostly work with smaller strings, then it shouldn't matter.

Taking each point into consideration, you should end up with this:

for (int i = 0; str[i] != '\0'; i++) {
    if (isalpha(str[i])) {
        str_san[k] = tolower(str[i]);
        k++;
    }
}

StackExchange Code Review Q#61730, answer score: 7