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\$
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
    \$\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\$ – JS1 Feb 16 '17 at 8:33
  • \$\begingroup\$ @JS1 Thanks, fixed. Shouldn't post an untested code under the influence. \$\endgroup\$ – vnp Feb 16 '17 at 17:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.