CODEWITHSUNDEEP
cc-strings
user2823183
user2823183

Reputation: 77

Get elements from string

I'm stumped in a rather trivial thing... So, basically I want the "words" between the first one and the last one to go to data and the last one to go to key.

C-POSIX only, pls.

Is strtok_r the way to go or I'm way off on this? Something else? 

char *key = NULL, *data=NULL, *save=NULL;
char comando[1024];
fgets(comando, 512, stdin);

strtok_r(comando, " ",&save);

while(strcmp(save,"\n")){
    strcat(data,strtok_r(NULL," ",&save));
}

key = strtok_r(NULL, "\n",&save);

P.S: comando is 1024 as memory is not a problem and better safe than sorry. fgets reads 512 'cause that's the char line limit on standard unix terminal.

Upvotes: 1

Views: 127

Answers (3)

JeremyP
JeremyP

Reputation: 86661

Lots wrong with your code

char *key = NULL, *data=NULL, *save=NULL;

Later on, you are using strcat to add strings to data but you have allocated no storage to data. That will cause a segmentation fault.

fgets(comando, 512, stdin);

fgets will read at most one less than the number passed to it. So, if the user does type in 512 characters, the string will have no terminating \n. Also, the only way to detect an error or end of file is to check the return result of fgets. If it's NULL either you have reached end of file (user has hit ctrl-d) or there is an error. In either case, the content of your buffer is indeterminate.

while(strcmp(save,"\n"))

I don't think you are allowed to rely on the assumption that your save pointer will point to the rest of the unconsumed string.

strtok_r(comando, " ",&save);

strtok_r signals that it has reached the end of the data by returning a NULL pointer. You can't throw away the return result without looking at it. Also, this will consume the trailing \n as part of the last token.

strcat(data,strtok_r(NULL," ",&save));

As I said before, data is a null pointer. Also, strtok_r can return NULL

I would do something more like:

char* currentTok = strtok_r(commando, " \n", &save); // separator is space or \n
char* previousTok = NULL;
while (currentTok != NULL)
{
    if (previousTok != NULL)
    {
        // save previousTok in data unless its the first token
    }
    previousTok = currentTok;
    currentTok = strtok_r(NULL, " \n", &save);
}
char* key = previousTok;

Upvotes: 1

Filipe Gonçalves
Filipe Gonçalves

Reputation: 21223

Your code will crash on this line:

strcat(data,strtok_r(NULL," ",&save));

Because you never reserved space for data. strcat will try to write to a NULL memory address.

Another thing to note is that you shouldn't rely on save to check for the end of the line. According to strtok's manpage:

The saveptr argument is a pointer to a char * variable that is used internally by strtok_r() in order to maintain context between successive calls that parse the same string.

Relying on the value of saveptr outside of strtok_r breaks the abstraction layer, you shouldn't assume anything about how strtok uses saveptr. It's bad practice.

A slightly better approach is to keep a pointer to the previous token returned by strtok, and a pointer to the current token. When strtok returns NULL, meaning there are no more tokens, then prev will point to the last token, which is your key. Here's some code:

char *key = NULL, *save=NULL;
char *prev, *curr;
char comando[1024];
char data[1024];

data[0] = '\0';
fgets(comando, 512, stdin);
prev = curr = strtok_r(comando, " ",&save);

while (curr != NULL) {
    prev = curr;
    curr = strtok_r(NULL, " ", &save);
    if (curr != NULL)
        strcat(data, prev);
}

key = prev;

Note that I allocated space for data by declaring it as array instead of pointer. The instruction

data[0] = '\0';

is there to make sure that strcat finds the null terminating byte in the first call.

You can replace the use of prev directly by key, I left it that way to make the code more readable.

A word of advice: always remember that strtok modifies its argument destructively (you lose the identity of the delimiting bytes), and that you can't call it with constant strings.

Note: data will contain every word concatenated. You lose the spaces. I'm not sure if this is what you want. If it's not, you might want to use something better than strcat (which is not very efficient, btw). For example, you code use sprintf to print the token into data with a leading space, and keep a pointer to the next free position in data.

Upvotes: 1

Michael
Michael

Reputation: 1503

I would suggest to replace your loop with a following code (printf() is used just for testing):

strtok_r(comando, " ", &save);
char *res = NULL;
while (NULL != (res = strtok_r(NULL, " ", &save))) {
  if (key != NULL) {
    //strcat(data, key); // FIXME
    printf("data = %s\n", key);
  }
  key = res;
}
printf("key = %s\n", key);

Also strcat() should not be used with NULL arguments - it leads to a crash. So data pointer should be pointing to some array. Results of the running of the code:

┌─(16:08:22)─(michael@lorry)─(~/tmp/strtok)
└─► gcc -o main main.c; echo "one two three four five" | ./main
data=two
data=three
data=four
key = five

Upvotes: 1

Related Questions