Finding occurrence of character in a string in C using pointers only

Question

Function find_any returns a pointer to the first occurrence of any member of the array whose first element is pointed to by pointer vals and has len number of elements in a half-open range of values. If none of the members of array vals are found, the function returns NULL.

This is the set of code that I've written so far but it does not seem to work. I already have a function that can find if a single character occurs in a string but I cant seem to make it check between more characters. I need to only use pointers and am not allowed to have subscripts or any include directives.

char const* find_any(char const *begin, char const *end, char const *vals, int len){
    int flag=0,i=0;
    while(begin!=end){
        while(i!=len){
            if(*begin==*vals){
                flag=1;
                break;
            }
            vals++;
            i++;
        }
        i=0;
        begin++;
    }
    if(flag==1) return begin;
    else return NULL;

You do know that `break` only breaks out of the inner `while` loop, right? So this function will always return `end` or `NULL` — UnholySheep, Oct 29 '22 at 17:35
Make the inner loop into a `for` loop: `for (int i = 0; i != len; i++)`. It makes the code easier to read when you under the clutter of the earlier declaration and the extra `i = 0;`. You could do `return begin;` instead of `flag = 1; break;`, which lets you lose the `flag` variable and an extra test (you simply return `NULL` if you exit the outer loop). — Jonathan Leffler, Oct 29 '22 at 17:35
However, your primary problem is that you increment `vals`, but you expect to iterate over the same string on the second character, and third…make a copy of `vals` for use in the loop. You are implementing a variant on [`strpbrk()`](https://pubs.opengroup.org/onlinepubs/9699919799/functions/strpbrk.html) — the difference is that you are not working with null-terminated strings but with 'pointer plus length/end' byte arrays. The fact that the `vals` value is not guaranteed to be null-terminated means you can't use `strchr()` and there isn't a `strnchar()` function available as standard. — Jonathan Leffler, Oct 29 '22 at 17:39
This looks like a very good time to learn how to [*debug*](https://ericlippert.com/2014/03/05/how-to-debug-small-programs/) your programs. For example by using a [*debugger*](https://stackoverflow.com/questions/25385173/what-is-a-debugger-and-how-can-it-help-me-diagnose-problems) to step through the code line by line while monitoring variables and their values. — Some programmer dude, Oct 29 '22 at 17:42
Also, when it comes to pointers I always recommend you use pen and paper to draw it all out. For example, draw rectangles representing the string you search, and the string of characters you want to find. Then draw arrows labeled `begin`, `end` and `vals` and point the arrows at their respective positions when the function is called. Then when you modify a pointer, erase and redraw the arrow representing the variable. If you do that you will find out one major problem with the code, quite quickly. — Some programmer dude, Oct 29 '22 at 17:44
thanks everyone for the replies, i'll definitely take in all the advice that I see and improve. — blzy, Oct 29 '22 at 19:31

Jonathan Leffler · Accepted Answer · 2022-10-29T20:16:31.517

Diagnosis

One of your primary problems is that you increment vals, but you expect to iterate over the same string multiple times: for the second character, and the third, and … You need to make a copy of vals for use in the loop.

You are implementing a variant on strpbrk() — the difference is that you are not working with null-terminated strings but with 'pointer plus length/end' byte arrays. The fact that the vals value is not guaranteed to be null-terminated means you can't use strchr() and there isn't a strnchar() function available as standard; otherwise you could use a function call in place of the inner loop.

Make both the loops into for loops: for (int i = 0; i != len; i++). It makes the code easier to read when you undo the clutter of the earlier declaration and the extra i = 0;. You should use return begin; instead of flag = 1; break;, which lets you lose the flag variable and an extra test (you simply return NULL if you exit the outer loop).

Another of your problems is that the break only exits the inner loop whereas you want to exit the outer loop too when you find the character. You could use a goto statement and a label, but that's worse than using a return in the middle of the function. Unless your tutor has a rule against return in the middle of a function, use it.

Fixed code

#include <stddef.h>

char const *find_any(char const *begin, char const *end, char const *vals, int len)
{
    for (const char *haystack = begin; haystack < end; haystack++)
    {
        for (const char *needle = vals; needle < vals + len; needle++)
        {
            if (*haystack == *needle)
                return haystack;
        }
    }
    return NULL;
}

Interface Consistency

I note that the interface uses two different methods for dealing with byte arrays that are not null terminated: pointers to the start and (one place beyond) the end, and a pointer to the start and the length. The interface should probably be more consistent — consistent code is easier to use. Either mechanism is valid, but use only one mechanism in a given function (and use it consistently in a suite of functions). Use either:

extern char const *find_any(char const *haystack, char const *h_end, char const *needle, const char *n_end);

or:

extern char const *find_any(char const *haystack, size_t h_len, char const *needle, size_t n_len);

or even:

extern char const *find_any(size_t h_len, char const haystack[h_len], size_t n_len, char const needle[n_len]);

Also, it's generally good practice to use size_t for sizes/lengths, rather than int (though I'm sure there are some who disagree). At the least, using size_t means you don't have to worry about checking for negative sizes or lengths.

Testing the code

The function is made static to avoid complaints from my default compiler options (source file ba59.c; program ba59). If the function were to be included in a library, there would be a header that declares the function (along with other functions) that would be included in both the source code that implements find_any() and in any code that uses the function.

gcc -O3 -g -std=c11 -Wall -Wextra -Werror -Wmissing-prototypes -Wstrict-prototypes -fno-common ba59.c -o ba59

Code:

#include <stddef.h>

static char const *find_any(char const *begin, char const *end, char const *vals, int len)
{
    for (const char *haystack = begin; haystack < end; haystack++)
    {
        for (const char *needle = vals; needle < vals + len; needle++)
        {
            if (*haystack == *needle)
                return haystack;
        }
    }
    return NULL;
}

#include <stdio.h>

static void test_find_any(const char *src, const char *end, const char *vals, int vlen)
{
    const char *rv = find_any(src, end, vals, vlen);
    if (rv == NULL)
        printf("No characters from [%.*s] were found in [%.*s]\n",
               vlen, vals, (int)(end - src), src);
    else
        printf("Found character from [%.*s] at [%.*s]\n",
               vlen, vals, (int)(end - rv), rv);
}

int main(void)
{
    char haystack[] = "Conniptions";
    char needle1[] = "ABXYZabxyz";
    char needle2[] = "ion";
    char needle3[] = "zap";

    char *h_end = haystack + sizeof(haystack) - 1;
    int n1_len = sizeof(needle1) - 1;
    int n2_len = sizeof(needle2) - 1;
    int n3_len = sizeof(needle3) - 1;

    test_find_any(haystack, h_end, needle1, n1_len);
    test_find_any(haystack, h_end, needle2, n2_len);
    test_find_any(haystack, h_end, needle3, n3_len);

    return 0;
}

Result:

No characters from [ABXYZabxyz] were found in [Conniptions]
Found character from [ion] at [onniptions]
Found character from [zap] at [ptions]

Finding occurrence of character in a string in C using pointers only

1 Answers1

Diagnosis

Fixed code

Interface Consistency

Testing the code