current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 4 down vote favorite

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.

share|improve this question
    
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. – Barmak Shemirani 2 days ago
    
you also can use std::regex for split string. – Kvaz1r 2 days ago
up vote 5 down vote accepted

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;
}
share|improve this answer
    
See here please how i use it prntscr.com/cnzg8d – Ramy 2 days ago
    
@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. – Loki Astari yesterday
up vote 4 down vote

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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