How to store elements of a string array into a struct in C?

Question

So I have a string: a = 2.b 1.d; milk cheese

I separate it by removing all the special characters (the equal sign, the period, the semicolon) and this is done using the strtok function in C. I take each element and store it into an array of strings, like this:

arr[0]="a"
arr[1]="2"
arr[2]="b"
arr[3]="1"
arr[4]="d"
arr[5]="milk"
arr[6]="cheese"

Now, I want to take those values and put them into a struct. Here is my struct:

struct stopPoints {
    int  weights[10];
    char connectingPoints[10];
    char *items;
    int startBool;
};

I've declared the struct and called it myPoint. Now, I want to store each of the separated elements up top into my struct. For example, i want to store the "2" and "1" into myPoint.weights[0] and myPoint.weights[1]. I want to store the "a" and "b" into myPoint.connectingPoints[0] and myPoint.connectingPoints[1].

I approached this by trying to distinguish between letters and numbers. I loop through the "arr" array and check if each index has either a letter or number. This is done by using the ASCII values (I know there's a better method to doing this according to the answers on my previous post). But when I try printing out the first weight element in my struct, I get a random value. How can I fix this? My code is below:

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

struct stopPoints {
    int  weights[10];
    char connectingPoints[10];
    char *items;
    int startBool;
};

int main ()
{
    struct stopPoints myPoint;
  char *arr[30];
  char str[] ="a = 2.b 1.d; milk cheese";
  char * pch;
  pch = strtok (str," ;=,.-");
  arr[0] = pch;
  int i=0;
  while (pch != NULL)
  {
    //printf ("%s\n",pch);
    pch = strtok (NULL, " ;=,.-");
    arr[i+1] = pch;
    printf("%s\n", arr[i]);
    i++;
  }
    int sizeofstring = sizeof(str)/sizeof(str[0]);

    int x,y=0;
     for (x=0; x<sizeofstring; x++){
          if (arr[y+1] >= 97 && arr[y+1] <= 122){
                myPoint.connectingPoints[x] = arr[y+1];
                y++;
          }
          else if (arr[y+1] >= 48 && arr[y+1] <= 57){
                myPoint.weights[x] = arr[y+1];
                y++;
          }
     }

    printf("%d\n", myPoint.weights[1]);

  return 0;
}

Answer 1

First, very good job on the detail, the minimum complete and verifiable example, and the formatting of your question. Next, it is apparent you are quite lost...

It is hard to imagine a more awkward approach to doing what you are attempting to do, but for the sake of learning, there is quite a bit of learning that can be had in doing it in an awkward fashion.

To begin, do not use magic numbers in your code. This is for readability and maintainability as your code grows. 10, 30, 97, 122, 48 and 57 are all magic numbers.

#define CPWT 10     /* if you need a constant, #define one (or more) */
#define NPTR 30     /*   (do not use "magic numbers" in your code)   */

Do not use magic numbers for characters. While you should use the macros provided in ctype.h for islower() and isdigit(), if you are going to use characters, then use characters, e.g. if (foo >= 'a' && foo <= 'z') not 97 and 122. Just single quote the character.

Next, you are overwriting every pointer in arr every time you assign arr[i+1] = pch; Why?

    char *arr[NPTR];                /* array of 30 UNINITIALIZED pointers */

While strtok does return a pointer, when you assign arr[i+1] = pch;, you are assigning the same pointer to each pointer in arr. When you are done, every element in arr will hold the last value returned by strtok (and since you assign after the last call to strtok returns NULL -- you most likely SegFault)

Further, before you "store" anything at the address of arr[0] to arr[NPTR-1], you must allocate storage based on the length of the string you are storing. (and yes, even though you are only storing a single-character in many elements, you are storing strings -- and every string requires a nul-terminating character).

You cannot assign strings in C (except for the assignment of string literals or during initialization of an array). Otherwise, you must copy strings in C.

So since you begin with uninitialized, unallocated pointers in arr, you must allocate and then copy to store information at each pointer address. You have two ways of doing so, (1) either malloc (length + 1) characters, validate the allocation, and then strcpy (or more efficiently memcpy since you have already scanned to find the nul-character with strlen()), or (2) if you have strdup() it will both allocate and copy as you did in (1) in a single function call. (note: since strdup allocates, you still must validate the allocation succeeded)

Example (1)

    if ((pch = strtok (str," ;=,.-")) != NULL) {    /* validate each call */
        size_t len = strlen (pch);                  /* get length */
        if ((arr[0] = malloc (len + 1)) == NULL) {  /* allocate/validate */
            perror ("arr[0]-malloc");
            exit (EXIT_FAILURE);
        }
        memcpy (arr[0], pch, len+1);    /* copy! string to arr[0] */
        i++;
    }

Example 2 (using strdup to allocate/copy)

    /* only loop if good return from strtok */
    while ((pch  = strtok (NULL, " ;=,.-")) != NULL)
    {
        /* allocate/copy all at once with strdup (if you have it) */
        if ((arr[i] = strdup (pch)) == NULL) {
            perror ("arr[n]-strdup");
            exit (EXIT_FAILURE);
        }
        i++;
    }

Since you have just filled i elements of arr, there is no need for:

    int sizeofstring = sizeof(str)/sizeof(str[0]);

You know you have i strings in arr -- use i not sizeofstring and step through each string you stored in arr, not each character in str. That defeats the entire purpose of having tokenized str. Further, you only want to consider single character or single digit strings in arr (not "milk" and "cheese") when setting connectingPoints and weights, so check whether the 2nd character is the nul-character, otherwise skip the element of arr.

You can't use y for both connectingPoints and weights, you have a, b, d connectingPoints (3 of them) and only 2 weights. You would be attempting to access outside the valid data in weights if you iterated from j = 0; j < y; ...

Again, you cannot assign a pointer as a character, so the easiest way to assign the 1st character in a string as a character, is simply to dereference the pointer, e.g. *arr[x] (which is equivalent to the character arr[x][0]). That in mind, you could do:

    /* i contains the number of strings in arr - use it */
    for (x = 0; x < i; x++) {
        if (arr[x][1] == 0) {   /* only consider single char strings in arr */
            if (cpts < CPWT && islower(*arr[x])) { /* check bound/lowercase */
                myPoint.connectingPoints[cpts] = *arr[x];  /* assign char */
                cpts++;
            }
            else if (weights < CPWT && isdigit(*arr[x])) { /* same w/digits */
                myPoint.weights[weights] = *arr[x];
                weights++;
            }
        }
    }

(note: the separate use of the counters cpts and weights instead of a single y)

Putting all the pieces together, you could do the awkward approach with something like the following:

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

#define CPWT 10     /* if you need a constant, #define one (or more) */
#define NPTR 30     /*   (do not use "magic numbers" in your code)   */

struct stopPoints {
    int  weights[CPWT];
    char connectingPoints[CPWT];
    char *items;
    int startBool;
};

int main (void)
{
    struct stopPoints myPoint = { .weights = {0} };
    char *arr[NPTR];                /* array of 30 UNINITIALIZED pointers */
    char str[] ="a = 2.b 1.d; milk cheese";
    char *pch;
    int i = 0, x, cpts = 0, weights = 0;

    if ((pch = strtok (str," ;=,.-")) != NULL) {    /* validate each call */
        size_t len = strlen (pch);                  /* get length */
        if ((arr[0] = malloc (len + 1)) == NULL) {  /* allocate/validate */
            perror ("arr[0]-malloc");
            exit (EXIT_FAILURE);
        }
        memcpy (arr[0], pch, len+1);    /* copy! string to arr[0] */
        i++;
    }

    /* only loop if good return from strtok */
    while ((pch  = strtok (NULL, " ;=,.-")) != NULL)
    {
        /* allocate/copy all at once with strdup (if you have it) */
        if ((arr[i] = strdup (pch)) == NULL) {
            perror ("arr[n]-strdup");
            exit (EXIT_FAILURE);
        }
        i++;
    }

    /* i contains the number of strings in arr - use it */
    for (x = 0; x < i; x++) {
        if (arr[x][1] == 0) {   /* only consider single char strings in arr */
            if (cpts < CPWT && islower(*arr[x])) { /* check bound/lowercase */
                myPoint.connectingPoints[cpts] = *arr[x];  /* assign char */
                cpts++;
            }
            else if (weights < CPWT && isdigit(*arr[x])) { /* same w/digits */
                myPoint.weights[weights] = *arr[x];
                weights++;
            }
        }
    }

    puts ("arr contents:");
    for (x = 0; x < i; x++)
        printf (" arr[%2d]: %s\n", x, arr[x]);

    puts ("\nconnectingPoints:");
    for (x = 0; x < cpts; x++)
        printf (" myPoint.connectionPoints[%2d]: %c\n", 
                x, myPoint.connectingPoints[x]);

    puts ("\nweights:");
    for (x = 0; x < weights; x++)
        printf (" myPoint.weights[%2d]: %c\n", 
                x, myPoint.weights[x]);

    return 0;
}

(note: the use of islower() and isdigit() from ctype.h)

Example Use/Output

$ ./bin/strtokarrptrs
arr contents:
 arr[ 0]: a
 arr[ 1]: 2
 arr[ 2]: b
 arr[ 3]: 1
 arr[ 4]: d
 arr[ 5]: milk
 arr[ 6]: cheese

connectingPoints:
 myPoint.connectionPoints[ 0]: a
 myPoint.connectionPoints[ 1]: b
 myPoint.connectionPoints[ 2]: d

weights:
 myPoint.weights[ 0]: 2
 myPoint.weights[ 1]: 1

Look things over and let me know if you have further questions.

Answer 2

I am not really understanding what you are trying to do, but arr is an array of pointers, myPoint.connectingPoints is an array of char and myPoint.weights is an array of int. So myPoint.connectingPoints[x] = arr[y+1] and myPoint.weights[x] = arr[y+1] will do pointer to char or int conversion, which is generally a bad idea.

Maybe you wanted to do

myPoint.connectingPoints[x] = arr[y+1][0];

and

myPoint.weights[x] = arr[y+1][0];

The same goes for the comparisons with arr[y+1]. 97, 122, 48 and 57 are not hardcoded memory addresses, are they?

This is totally ignoring the fact that you have a buffer overflow in myPoint.connectingPoints and myPoint.weights. Hint: sizeofstring is larger than 10.

Please, always compile with -Wall. The flag exists for a reason

Answer 3

You do not initialize your struct at any point and you alternate by assigning values to myPoint.connectionPoints[x] and myPoint.weights[x] but then skip a spot in the other array so that is the reason you get random values

      if (arr[y+1] >= 97 && arr[y+1] <= 122){
            myPoint.connectingPoints[x] = arr[y+1];  // myPoint.weights[x] left unassigned 
            y++;
      }
      else if (arr[y+1] >= 48 && arr[y+1] <= 57){
            myPoint.weights[x] = arr[y+1]; // myPoint.connectingPoints[x] left unassigned
            y++;
      }

I would suggest you convert your values to the struct already when you get the return value from strtok to avoid allocating for things you do not later need.

E.g.

    int index = 0;
    for (char* pch = strtok(str, " ;=,.-"); pch != NULL; pch = strtok(NULL, " ;=,.-")
    {
      switch (index++)
      {
      case 0:
      case 1:
      case 2:
      ...
      default:
        break;
      }
    }

EDIT: in fact I am not sure you need the switch at all, just have a counter for each array and increment when you assign to it.

In the case of the return value of strtok() you must take care to allocate memory for the return value in order to store the value between each iteration since it uses a static buffer of the original string when it returns tokens, but if you directly convert the token you don't need to.

When you check the value of the character use one of the standard c runtime functions for that purpose isdigit() isalpha()/islower() instead of checking ASCII value directly.