HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Tokenize a string

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
tokenizestringstackoverflow

Problem

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.

Solution

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;
}

Code Snippets

// 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;
}

Context

StackExchange Code Review Q#142820, answer score: 5

Revisions (0)

No revisions yet.