2
\$\begingroup\$

I wrote this function which does the following from scratch w/o looking for ways that it is typically done:

  1. Accepts a char c to search for.
  2. Accepts a char* str to search within.
  3. Accepts a buffer pointer, and stores all indices of c that are in str inside the provided buffer.
  4. Returns the # of times that c was found in str.

Please critique and also - Are there better alternatives to export the indices other than using a buffer ptr?

#include <stdlib.h>
#include <string.h>
#include <stdio.h>
int str_contains(const char c, const char* str, int* output_buffer){
    int i, str_len;
    str_len = strlen(str);
    int *holder = malloc(str_len*sizeof(int));
    int holder_iteration = 0;
    for(i = 0; i < str_len; i++){
        if(str[i] == c){
            holder[holder_iteration++] = i;
        }
    }
    memcpy(output_buffer,holder,holder_iteration*sizeof(int));
    free(holder); 
    return holder_iteration;
}

int main(){
    char* myString = "Welcome to the jungle, baby. Do you like to juggle or jump around?";
    int* output = malloc(500*sizeof(int));
    int number = str_contains('a',myString,output);

    printf("# of times: %d\nIndices: \n",number);
    int i;
    for(i = 0; i < number; i++){
        printf("%d\n",output[i]);
}

free(output);
    return(EXIT_SUCCESS);
}
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$
  • The final memcpy may overflow output_buffer. I recommend to change the signature to

    int str_contains(const char * str, char c, int *indices, size_t indices_size);
    

    The caller shall compare the return value with the size of buffer it passed and act accordingly.

  • Dynamic memory is uncalled for. You may safely collect indices directly into the passed buffer.

  • There is no need for strlen.

  • When appropriate, use pointers.

All that said,

int str_contains(const char * str, char c, size_t * indices, size_t indices_size)
{
    size_t occurrences = 0;
    for (char * cursor = str; *cursor; cursor++) {
        if (*cursor == c) {
            if (occurrences < indices_size) {
                indices[occurrences] = cursor - str;
            }
            occurrences++;
        }
    }
    return occurrences;
}
\$\endgroup\$
2
  • 2
    \$\begingroup\$ I like everything except the fact thatindices[0] is never filled in because you incremented occurrences beforehand. Also, indices_size shouldn't be a pointer type (your explanation was right but your code was not). \$\endgroup\$ Commented Feb 16, 2017 at 8:33
  • \$\begingroup\$ @JS1 Thanks, fixed. Shouldn't post an untested code under the influence. \$\endgroup\$ Commented Feb 16, 2017 at 17:57

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.