Using strchr() to count occurrences of a character in a string

Question

I'm almost finished with the class semester, and I'm working on an assignment to write a function to find the number of a certain character in a string, given the function prototype the teacher assigned. I know I must be doing something stupid, but this code is either locking up or looping indefinitely in my function.

It is an assignment, so I'm not looking for anyone to do my homework for me, but merely to point out where I'm wrong and why, so I can understand how to fix it. I would appreciate any help that you are willing to give.

Here's the code I've written:

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

int charCounter(char* pString, char c);

int main(void)
{
    char* inpString = "Thequickbrownfoxjumpedoverthelazydog.";
    int charToCount;
    int eCount;

    eCount = 0;
    charToCount = 'e';
    eCount = charCounter(inpString, charToCount);
    printf("\nThe letter %c was found %d times.", charToCount, eCount);

    return 0;
} // end main

int charCounter(char* pString, char c)
{
    int count = 0;
    char* pTemp;

    do
    {
        pTemp = strchr(pString, c);
        count++;
    }
    while(pTemp != NULL);

    return count;
} // end countCharacter

Answer 1

Your loop is always looking from the beginning of pString, and is always finding the first 'e' over and over again.

If you declare char* pTemp = pString; then you can iterate a little differently (I pasted the wrong version earlier, sorry!):

char* pTemp = pString;

while(pTemp != NULL)                                                                                                    
{                                                                                                                       
    pTemp = strchr(pTemp, c);                                                                                                           
    if( pTemp ) {
        pTemp++;
        count++;
    }                                                                                                
}

This forces pTemp to point just after the character you just found before looking for the next one.

It would be easier to just do:

char* pTemp = pString;
while( *pTemp )
    if( *pTemp++ == c) count++;

Okay, after thinking about it, even after you already have this working, I changed the inner loop to a form I am more happy with:

while( (pTemp = strchr(pTemp, c)) != NULL) {                                                                                                                       
   count++;                                                                                                             
   pTemp++;
}

Answer 2

You are always restarting from the beginning. No wonder you never come to an end.

char* pTemp; // Init here = pString
do {
    pTemp = strchr(pString, c); // pString? Really? Should be pTemp
    count++;
} while(pTemp != NULL); // pTemp != NULL is verbose for pTemp here

Still, better avoid the library function and do a direct loop over all elelemts.

Just to jump on the wagon train:

size_t count(const char* s, char c) {
    size_t r = 0;
    for (; *s; ++s)
        r += *s == c;
    return r;
}

If you insist on using strchr():

size_t count(const char* s, char c) {
    size_t r = 0;
    while ((s = strchr(s, c)))
        ++r;
    return r;
}