Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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 <stdio.h>
#include <stdlib.h>
#include <string.h>

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] <= 'Z') {
            str_san[k] = str[i] + 32; //changes uppercase to lowercase
            k++;
        }

        if (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;
}
share|improve this question
2  
A reminder of the difference between C and C++: in C, you don't have to cast the return of malloc. Many C devs actually consider these casts to be both pointless and wrong (they hide errors, and are sometimes used to stomp out compiler warnings, which in turn causes bugs) –  Elias Van Ootegem Sep 3 at 15:09

3 Answers 3

up vote 7 down vote accepted

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] <= 'Z') {
            str_san[k] = str[i] + 32; //changes uppercase to lowercase
            k++;
        }
    
        if (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] <= 'Z') {
            str_san[k] = str[i] + 32; //changes uppercase to lowercase
            k++;
        }
    
        if (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++;
        }
    }
    
share|improve this answer
    
Alright, I updated the code in an edit to my original post. I implemented two functions and simplified the logic by using islower() and isalpha() instead of the previous ridiculousness. I'm still unsure about the dynamic memory stuff. What is the best way to allow an arbitrarily large input string? I was thinking about setting the char array size to its maximum and then using realloc() after getting the input string from the user to bring it down to a manageable size, but I'm not sure that makes any sense. –  Thomas Sep 2 at 3:04
    
@Thomas: I've had to rollback your post as the original code shouldn't be updated based on answers (more info here). As for your question, I may have to look at it a bit more, or someone more knowledgeable with C could post an answer about it. You could also post a follow-up question that incorporates my advice, and with more emphasis on memory-management. –  Jamal Sep 2 at 3:09

Overall, it's very good for a refresher. It's clear and easy to understand. In addition to what Jamal has said though, I have a few things to add.


You have to be very careful with scanf and %s. What happens if the user inputs something longer than 50 characters (well, really 49)? scanf will just keep on writing. As Loki Astari pointed out, this requires a single word (well, a single sequence of non-whitespace) to be incredibly long, but we typically shouldn't assume users are providing sane input.

In short, what I'm saying is that you have a potential buffer overflow. You need to either constrain the length of the input (scanf("%49s", str)), or use a different method of reading the input. If you stick to standard C, dynamic memory allocation is going to be involved in some kind of loop. If you break standard compliance, you can get away with a bit easier method.

If you're interested in standards compliance, you will want to look into fgets, and realloc. If you don't care being tied down to GNU C, see this.

I would suggest that you go the standard compliant way and just put into a function so you only ever have to write it once. That way you don't have to needlessly break from the standard, but it remains easy to use.


//make str_san as small as possible

I'm being super picky now, but that's technically a lie. strlen(str) + 1 is not guaranteed to be the shortest that str_san can be. A more accurate comment might mention that it's the shortest buffer that can be allocated without first counting how many elements are going to be copied into it.


The while loop for copying str into str_san is better fit (in my opinion) for a for loop.


I wouldn't reuse len but would instead have a second variable (san_len or something).

share|improve this answer
    
It would have to be a long word to be greater than 50 characters. :-) You may want to mention the %s stops at white space. –  Loki Astari Sep 2 at 23:39
    
@LokiAstari Users are insane though :p. And good point about whitespace. Will edit. –  Corbin Sep 2 at 23:54

Pretty fine as a starter, but I feel there are some areas on we can focus:

  1. Whenever we do know the size of data buffer predefined, it is prefer to use the char buffer rather than declaring a pointer and dynamically allocating.

    I prefer to use

    char str[50] = "";
    

    instead of

    char *str = (char *)malloc(50);
    
  2. I feel it is a good practice to initialize the dynamically allocated memory.

    For example, after

    str_san = (char*)malloc(strlen(str) + 1);
    

    I would initialize the allocated memory as

    memset(str_san, 0, strlen(str) + 1);
    

    This will minimize your effort to set the last character to NULL.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.