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

Must you use `strchr`? I would do a straight iteration over all elements. — Deduplicator, Apr 24 '14 at 19:58
Using `strchr` for this is an "overkill". Simply iterate the input string until you reach a null-character, and increment the counter whenever you encounter the input character. — barak manos, Apr 24 '14 at 19:59
Change it to strchr(pTemp, c) right now you start from the beginning of the original string all the time — Gábor Buella, Apr 24 '14 at 19:59
Consider going for `const`-correctness: While C string literals are of type `char[]` for historical reasons, they are actually immutable. — Deduplicator, Apr 24 '14 at 20:05
@Deduplicator I'm assuming that's what he wants used because the chapter is all about these string functions. I'd have an easier time just iterating through the string a character at a time. :-( BuellaGábor and Deduplicator, thanks for the good advice. — David Peterson Harvey, Apr 24 '14 at 20:17
@DavidPetersonHarvey - the consensus here (notwithstanding minor variations in implementation) seems to be that you need to iterate through the string, looking at each `char` element of the string array to determine if that element is `==` to your `c`. `strchr()` will not do that for your easily. (without undue manipulation of your string each time) — ryyker, Apr 24 '14 at 20:24
Always remember, non-zero is true. Thus, avoid `!=0`, `!=NULL` or `!='\0'` in a conditional context. It is superfluous. For the same reason, use `!exp` for the opposite. — Deduplicator, Apr 24 '14 at 20:24
Thanks. I think I'll give up and just iterate through the thing, hoping for the best at grade time. :-) — David Peterson Harvey, Apr 24 '14 at 20:26

JohnH · Accepted Answer · 2014-04-24T20:46:34.193

5

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++;
}

edited Apr 24 '14 at 20:46

answered Apr 24 '14 at 19:59

JohnH

2,713
12
21

I love your second suggestion but the chapter is all about the use of the functions. The first answer gives a segmentation fault and dumps the core but iterating pString gets closer. However, I need to figure out how to iterate farther after the character is found because I'm getting a lot of duplicate counts. Thanks for getting me closer. – David Peterson Harvey Apr 24 '14 at 20:21
Did you remember to put `char* pTemp = pString;` instead of `char* pTemp;` before my first block? :) I actually tested the block before posting it... – JohnH Apr 24 '14 at 20:23
Yes, and it still kicked out on me. I even tried copying and pasting your code when all else failed, in case I made some stupid little mistake. I'm compiling on an Ubuntu machine, if that makes a difference. – David Peterson Harvey Apr 24 '14 at 20:27
1

@DavidPetersonHarvey -- I pasted the wrong version earlier, sorry. :( – JohnH Apr 24 '14 at 20:35
1

Ooh, John, awesome! It just needed one small change. I put count++ with pTemp++ in a block under the if statement so it only incremented when the character was actually found. That prevented it from automatically counting the extra time before the first character was found. Perfect! Thank you! Thanks to everyone here, I understand much better how it works! – David Peterson Harvey Apr 24 '14 at 20:42
I just re-wrote the loop to a form I am even more happy with and added it at the very end of my post... – JohnH Apr 24 '14 at 20:47
@JohnH - regarding the last edit, it looks like `count` will increment each time `pTemp` does. How will that result in an accurate count of `c`? (I see it does, just not sure how) – ryyker Apr 24 '14 at 20:58
1

If, and only if, pTemp wasn't null, we enter the body of the while..loop. This means we found the char, so we increment count. Since we then want to look for the next character, we increment pTemp to point to the character after the one we just found... and then the top of the while..loop goes and searches for it with the next call to strchr() starting one char after the one we just found. If it finds another, we enter the body of the while loop again, if not, we don't re-enter it. – JohnH Apr 24 '14 at 21:01

Deduplicator · Answer 2 · 2020-09-23T12:19:37.360

3

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;
}

edited Sep 23 '20 at 12:19

answered Apr 24 '14 at 19:59

Deduplicator

44,692
7
66
118

I would love to do that but this chapter is all about the string functions. (sigh) – David Peterson Harvey Apr 24 '14 at 20:24
far better and quicker method! – Dandymon Sep 03 '17 at 18:05

ryyker · Answer 3 · 2017-08-02T12:48:24.563

1

strchr() is always looking in the same place, it will not progress through the string...

Try this modification to traverse through the string using length of string, and a simple char comparison:

int i, len = strlen(pString);
count = 0;
for(i=0;i<len;i++)
{
    if(pString[i] == c) count++; //increment count only if c found
}

return count;

Without using strlen() (to address comment)

i=-1, count = 0;
while(pString[++i])
{
   if(pString[i] == c) count++;
}  
return count;

edited Aug 02 '17 at 12:48

answered Apr 24 '14 at 20:01

ryyker

22,849
3
43
87

2

why use strlen()? That adds an additional iteration through the string just to get its length. Delete variable `len` entirely and instead of `i – JohnH Apr 24 '14 at 20:07
1

@JohnH - Using `strlen()` is just one method. It works, so does yours. I am not trying to break the land speed record here, just answer the question. :) (See edit) – ryyker Apr 24 '14 at 20:12

score 1 · Answer 4 · answered Apr 24 '14 at 20:03

strchr returns a pointer to the first occurrence of the character c in pString.

So if you give a pointer to the beginning of your string in each loop, pTemp will always have the same value and never be NULL if the character c exists. That's why you have an infinite loop.

You might want to do some pointer arithmetic to solve your problem here ;)

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

4 Answers4