Tokenize a string

Question

I used strtok to tokenize a string. But many developers said me to avoid strtok, why? How should I optimize this code? Is it really wrong that I used strtok? If so then with what should I replace it?

DWORD FN_attack_speed_from_file(const char *file)
{
 FILE *fp = fopen(file, "r");
 if(NULL == fp)
 {
 return 0;
 }
 int speed = 1000;
 const char *key = "DirectInputTime";
 const char *delim = " \t\r\n";
 const char *field, *value;
 char buf[1024];
 while(fgets(buf, 1024, fp))
 {
 field = strtok(buf, delim);
 value = strtok(NULL, delim);
 if(field && value)
 {
 if(0 == strcasecmp(field, key))
 {
 float f_speed = strtof(value, NULL);
 speed = (int)(f_speed * 1000.0);
 break;
 }
 }
 }
 fclose(fp);
 return speed;
}

I would like to modernize this as C++, and learn modern coding.

• Barmak Shemirani: strtok is a valid function in C++. It's not wrong to use it in C++, it's not necessarily that much faster or slower. But your C++ code will look better if you avoid strtok. If you want to move to C++ then it's more important to use std::fstream, std::getline and std::string which have significant advantage over their C counterpart, you would end up avoiding strtok as well.
• user110702: you also can use std::regex for split string.

Answer 1

I used strtok to tokenize a string. But many developers said me to avoid strtok, why?

Because strtok() modifies the input is the main reason.
But it has other issues that make it not a good option in modern code.

How should I optimize this code?

Well let us make it correct first before we go for optimizations. But strtok() will probably be the fastest. That's because it basically does nothing but tokenization. But this also makes it dangerous to use.

Is it really wrong that I used strtok?

Yes if you are using C++. The better technique is to code for type safety and re-usability. The difference in speed will be minimal. So small that if this program is running for years that even the cumulative time saves by strtok() will not amount to the time taken to fix a single bug by a human.

If so then with what should I replace it?

You should probably use the C++ stream operators.

C++ Version

 // Slightly different to the original.
 // If the value part has an illegal character for a float it
 // will probably mess up.
 DWORD FN_attack_speed_from_file(char const* fileName)
 {
 std::ifstream file(fileName);
 std::string field;
 float value;
 while(file >> field >> value)
 {
 if (field == "DirectInputTime") {
 return value * 1000.0;
 }
 }
 return 1000;
}

• Ramy: See here please how i use it prntscr.com/cnzg8d
• Loki Astari: @Ramy: My code is going to fail on that because it will not correctly handle the line {. But I am sure you can take what I have given you and extend it to your needs.

Answer 2

Since you have indicated that you are using C++, you should use idiomatic C++ approaches to parse input. In particular, you should avoid strtok(), which is not recommended even in C, since it is not thread-safe.

It appears that you are looking to read whitespace-delimited words and floats. For that, use the >> operator on an ifstream.