3
\$\begingroup\$

I have written a program to parse a string depending on the string the user provides. Any advise/criticism is appreciated.

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

#define DEFAULT_SIZE 10
#define RESIZE_MULTI 2

/*compares value to see if it exists in parse. return 0 if exists*/
static uint8_t compare(const char value, const char *parse)
{
   size_t i;

   i = 0;
   while(*(parse + i) != '\0')
   {
      if(*(parse + i) == value)
         return 0;
      i++;
   }
   return 1;
}

/*string must be null terminated. the string that needs to be split*/
/*parse must be null terminated. contains value that the string will be split at*/
/*matrix contains all the string that was split based on parse. the function return how how many string there are*/
size_t parse_string(const char *string, const char *parse, char *** const user_matrix)
{
   /*how many spots there are*/
   size_t temp_matrix_size;
   /*how many acctually string there are*/
   size_t temp_matrix_length;
   char **temp_matrix;
   size_t i;
   size_t counter;

   /*allocate initial memory. array of pointers*/
   temp_matrix = malloc(sizeof(*temp_matrix) * DEFAULT_SIZE);
   if(temp_matrix == NULL)
      goto FAIL0;
   temp_matrix_size = DEFAULT_SIZE;
   /*since right now there 0 string aka pointers to strings*/
   temp_matrix_length = 0;


   i = counter = 0;
   while(*(string + i) != '\0')
   {
      /*resize array of pointers*/
      if(temp_matrix_length >= temp_matrix_size)
      {
         char **temp = realloc(temp_matrix, sizeof(*temp_matrix) * temp_matrix_size * RESIZE_MULTI);
         if(temp == NULL)
            goto FAIL0;
         temp_matrix = temp;
         temp_matrix_size = temp_matrix_size * RESIZE_MULTI;
      }

      if(compare(*(string + i), parse) == 0)
      {
         char *temp;
         if(counter > 0)
         {
            /*need the plus 1 for terminating char*/
            temp = malloc(sizeof(*temp) * (counter + 1));
            if(temp == NULL)
               goto FAIL0;

            /*setting terminating char*/
            *(temp + counter) = '\0';
            /*copy over the string before the parsing char*/
            memcpy(temp, string + (i - counter), counter);

            /*store the string*/
            *(temp_matrix + temp_matrix_length) = temp;
            temp_matrix_length++;
         }

         /*store only the parsing char with null terminated char*/
         temp = malloc(sizeof(*temp) * 2);
         if(temp == NULL)
            goto FAIL0;
         *(temp + 0) = *(string + i);
         *(temp + 1) = '\0';

         *(temp_matrix + temp_matrix_length) = temp;
         temp_matrix_length++;

         i++;
         counter = 0;
      }
      else
      {
         i++;
         counter++;
      }
   }

   /*special case*/
   if(counter != 0)
   {
      char *temp = malloc(sizeof(*temp) * (counter + 1));
      if(temp == NULL)
         goto FAIL0;

      /*setting terminating char*/
      *(temp + counter) = '\0';
      /*copy over the string before the parsing char*/
      memcpy(temp, string + (i - counter), counter);

      /*store the string*/
      *(temp_matrix + temp_matrix_length) = temp;
      temp_matrix_length++;
   }

   *user_matrix = temp_matrix;

   return temp_matrix_length;

   FAIL0:
   if(temp_matrix != NULL)
   {
      /*free all the strings*/
      for(i = 0; i < temp_matrix_length; i++)
      {
         free(*(temp_matrix + i));
      }
      /*free the array of pointers*/
      free(temp_matrix);
   }
   *user_matrix = NULL;
   return 0;
}

int main(void)
{
   char **matrix;
   size_t size;

   /*can have anything you want*/
   char *str = "Hello Word!!!";
   /*can have any char you want*/
   char *parse = "! ";

   size = parse_string(str, parse, &matrix);

   for(size_t i = 0; i < size; i++)
   {
      printf("%s\n", *(matrix + i));
   }

   for(size_t i = 0; i < size; i++)
   {
      free(*(matrix + i));
   }
   free(matrix);

   return 0;
}
\$\endgroup\$
5
\$\begingroup\$
  • Major remark of the code as whole: for some reason you write the unreadable version of array access *(arr+i) all over the place. Don't do that! This makes your code look needlessly obscure and hard to read. Instead use the much more readable arr[i].

  • Your compare is just a naive implementation of strchr. It would be much more efficient to use strchr.

  • Don't write while loops when you can write for loops, the latter is easier to read and often optimizes better too.

  • There is no point in const qualifying parameters passed by value - it only confuses the caller. Though this is a bit subjective and coding style.

  • The "correct naive" implementation of strchr would rather look like this:

      static const char* compare (char value, const char *parse)
      {
        for(size_t i=0; parse[i] != '\0'; i++)
        {
          if(parse[i] == value)
          {
            return &parse[i];
          }
        }
        return NULL;
      }
    
  • Since you don't use the above version but just return 1 or 0, you end up calling compare over and over on the same data, which is inefficient. If you had a version returning a pointer or index, you could continue searching from that location at next iteration, instead of starting over.

  • You can get rid of the "on error goto" pattern and leave all clean-up to a wrapper function. It's a bit more readable and you don't have to suffer the old "goto considered harmful" debate yet again:

      size_t parse_string (/* params */)  // wrapper function
      {
        // declare all resources potentially needing clean-up here:
        char **temp_matrix = NULL;  
        ...
    
        bool ok;
        ok = actual_function (/* params */); // the algorithm, private static inline
    
        if(!ok)
        {
          if(temp_matrix != NULL)  // the clean-up code from your FAIL0 label here
          ...
        }
      }
    

    Upon failure, "actual_function" just return false; and then the wrapper function can worry about clean-up.

    (For more intricate scenarios like some opaque struct allocated by your lib, you can move the whole clean-up code into yet another public "destructor" function.)

  • Generally, more than 2 levels of indirection is bad practice, but your scenario here is one of the very few cases where three *** is actually motivated. However, char *** const means that the outer-most pointer is const-qualified, which is just weird. The caller doesn't care if you change that local variable internally or not, it just serves to confuse the user. Again, just drop const of parameters passed by value. In this case the char*** passed by value.

  • Don't name your variables with identifiers used by C++ keywords, functions, libraries etc. Like string. Even if you never intend to use the code with C++, many IDE use same code formatting and color coding for C and C++, like this site for example.

\$\endgroup\$
3
  • \$\begingroup\$ I had a question related to this "Since you don't use the above version but just return 1 or 0, you end up calling compare over and over on the same data, which is inefficient. If you had a version returning a pointer or index, you could continue searching from that location at next iteration, instead of starting over.". My question is lets say if the user provided a 'space char to be parsed'. Then the 'string to be parsed' has multiple spaces then would I not need the pointer to the beginning of the parse string? And I apologize If I did not make it clear in my original code. Thank you! \$\endgroup\$
    – Dagar
    Apr 15 at 12:15
  • 1
    \$\begingroup\$ @Dagar It's not really clear what you mean by "parse", since the meaning of the term is very broad and just means "go through a chunk of text". But if you have multiple spaces and want them to act as separators between tokens, then you wouldn't start over each time. If the task is to find all the spaces, you wouldn't start over either, but continue searching from the last found one + 1. \$\endgroup\$
    – Lundin
    Apr 15 at 13:24
  • \$\begingroup\$ OP's compare() differs from strchr() when value == 0. This answer's compare() mis-matches strchr() like-wise. Overall code might not use value == 0, so of minor consequence. \$\endgroup\$ Apr 18 at 12:37
3
\$\begingroup\$
  • compare is a dubious name. The purpose of the function is to tell whether parse string contains a character. Consider

      bool contains(char * str, char ch);
    
  • The special case does not check for a possible overflow. temp_matrix_length could reach temp_matrix_size by the time it is executed.

  • I do not endorse a pointer notation here. string[i] is easier to read than *(string + i).

  • Code feels overcommented. Every time you feel compelled to put a comment like

      /* resize array of pointers */
    

    you admit that you failed to express the intention in the code itself. Make it a function and give it an expressive name:

      temp_matrix = resize(temp_matrix, new_size);
    
  • The for loop seems more natural than while. In any case, lift i++ out of the if/else block.

  • Flat is better than nested:

      if (!contains(parse, string[i]) {
          counter++;
          continue;
      }
      // proceed with adding substrings
    
  • DRY. The code to store the string is repeated 3 times. Factor is out into a function.


With all the bullets above, consider

    for (i = 0; string[i] != 0; i++) {
        if (!contains(parse, string[i]) {
            counter++;
            continue;
        }
        if (counter > 0) {
            temp_matrix = add_string(temp_matrix, string + i - counter, counter);
        }
        temp_matrix = add_string(temp_matrix, string + i, 1);
        counter = 0;
    }
    temp_matrix = add_string(temp_matrix, string + i - counter, counter);

where add_string would take care about resizing temp_matrix.

\$\endgroup\$
1
\$\begingroup\$

In addition to the other answers:

  • strpbrk or strcspn are useful for finding the next delimiter in a string.
  • strspn can be used to find the next character that isn't a delimiter in a string.

So given a char const* str as input, we could do something like:

while (*str)
{
    char const* end = strpbrk(str, delimiters);
    if (!end) end = strchr(str, '\0'); /* no delimiter found - use end of str */
    
    /* ... copy string in range [`str`, `end`) to output */

    str = end;
    str += strspn(str, delimiters); /* skip contiguous delimiters */
}
\$\endgroup\$

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.