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.

Write an efficient function to find the first non-repeated character in a string. For example, the first non-repeated character in "total" is 'o' and the first non-repeated character in "teeter" is 'r'. Please see my solution, give me some feedback and suggestions for improving and optimizing it if needed. Thank you!

#include <stdio.h>
#define SIZE 100                /* max size of input string */
#define LIM 5                   /* max number of inputs */
char repeat_ch(const char *);   /* return first non-repeated character */
int main(void)
{
    char line[SIZE];
    int i = 0;
    char ch;

    while(i < LIM && gets(line))
    {
        ch = repeat_ch(line);
        if(ch != NULL)
            printf("1st non-repeated character: %c\n", ch);
        else
                printf("There is no unique character in a string: %s\n", line);
        i++;
    }

}

char repeat_ch(const char *string)
{
    char array[130] = {0};
    char *p = string;

    /* store each character in array, use ascii code as an index for character
     * increment each time the same character appears in a string
     */
    while(*p)                               // stop when '\0' encountered
    {
        array[*p]+=1;
        p++;
    }
    while(*string)
    {
        if(array[*string] == 1)
            return *string;         // stop when unique character found
        string++;
    }

    return *string;
}
share|improve this question
add comment

1 Answer

up vote 3 down vote accepted

Alex, it looks quite efficient. There are some issues that I see:

  • gets should never be used. Use fgets (but note that you will have to strip the trailing \n)

  • define main at the end to avoid the need for a prototype for repeat_ch

  • declare repeat_ch as static

  • your limit does not work as i is not incremented. But why not stop on reading an empty string rather than limit the number of loops?

  • NULL is normally defined as (void *) 0 so comparing a char with NULL is wrong. The compiler will warn you of that. Just use 0 or better '\0'

In repeat_ch

  • function name is inaccurate - function looks for a non-repeated char.

  • the two // comments are noisy (ie. don't tell reader anything)

  • array would be better sized 256

  • p should be const

  • for-loops would be better

    for (const char *p=string; *p; ++p) {
        array[*p] += 1;
    }
    for (const char *p=string; *p; ++p) {
        if (array[*p] == 1) {
            return *p;
        }
    }
    return '\0';
    
share|improve this answer
    
thanks for feedback was really useful. Have added "i" as it was my copy-paste typo. –  alex Dec 31 '12 at 15:20
    
+1: Very good analysis. The only things I can see that you didn't mention are trivia: (1) erratic indentation in main() and (2) use of if(...) etc instead of if (...). The omitted return 0; from main() is OK in C99 or later, though personally I think that rule was a mistake and always include it. –  Jonathan Leffler Jan 1 '13 at 7:16
add comment

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.