6
\$\begingroup\$

I'm trying to convert an std::string to an int with a default value if the conversion fails. C's atoi() is way too forgiving and I've found that boost::lexical_cast is quite slow in the case where the cast fails. I imagine it's because an exception is raised. I don't have C++11, so stoi() is out.

The Delphi function StrToIntDef is the ideal, and it's also available in C-Builder (where I'm working currently). But I want something more portable that works with std::string.

I've created the following function, which uses atoi() but first tests for error conditions. It also allows for leading and trailing spaces, which are harmless in my situation.

int stringtoIntDef(const std::string & sValue, const int & DefaultValue) {
// convert a std::string to integer with a default value returned
// in the case where the string doesn't represent a valid integer 
// - accepts leading or trailing spaces as valid

   bool hasDigits = false;
   bool TrailingSpace = false;
   for (std::string::size_type k = 0; k < sValue.size(); ++k) {
      if ((sValue[k] == ' ') || (sValue[k] == '\t')) {
         TrailingSpace = hasDigits;
      } else if ((sValue[k] == '0') || (sValue[k] == '1') || (sValue[k] == '2') || (sValue[k] == '3') ||
                  (sValue[k] == '4') || (sValue[k] == '5') || (sValue[k] == '6') || (sValue[k] == '7') ||
                  (sValue[k] == '8') || (sValue[k] == '9')) {
         if (TrailingSpace) {
            return DefaultValue;
         } else {
            hasDigits = true;
         }
      } else if ((sValue[k] == '-') && !hasDigits) {
         hasDigits = true; // this protects against "--"
      } else {
         return DefaultValue;
      }
   }
   return atoi(sValue.c_str());
}

In my testing, I've compared it against StrToIntDef and it is just as fast, but lexical_cast is much slower in the case where the default is returned. For 1000 iterations, lexical_cast took 5 seconds while the other 2 weren't measurable.

For my lexical_cast function, I've used this:

try {
    return boost::lexical_cast<int>(sValue);
}
catch (boost::bad_lexical_cast &) {
    return DefaultValue;
}

Are there any gotchas I might have missed here?

\$\endgroup\$
  • 1
    \$\begingroup\$ Hi, regarding this suggested edit, feel free to add your bullet-proof solution as another answer - editing others' content is fine, but not for commenting and/or appending to their posts. \$\endgroup\$ – Mathieu Guindon Nov 15 '13 at 16:45
4
\$\begingroup\$

This is a follow up to your second follow up. I just have a few comments, and you have two bugs.

  • stringtoIntDef is a strange name. In particular, why are Int and Def capitalized but not To? I would go either full camelCase, or I would use the standard library convention of under_scores.
  • You never delete[] s which means you have a memory leak.
  • I actually meant in my comment that you could check the end pointer from strtol to see if all that follows is either nothing or 0. I should have been more specific. An example of this is below.
  • If cstdlib was included (as it should be) rather than stdlib.h, then strtol needs to be std::strtol. The cheader-style headers guarantee declarations in the std namespace, but they do not guarantee declarations in the global namespace. (Every C++ implementation I've ever seen does both, but they not required to do the global namespace.)
  • Define variables as late as possible. In other words, declare result when you're defining it to be strtol's return value.
  • If you don't want a variable to change after its initial value, make it const. This keeps you from accidentally changing the value, and it makes your intentions more clear to anyone who inherits the code.
  • You must provide a base of 10 to strtol, not 0. 0 is the default and it means to try to parse a number as base 10 unless it's prefixed with either 0x (for hex) or 0 (for octal). Assuming you want just decimal, it must be 10.
  • If you do want the default behavior, don't put the 0. Just let it default to 0. Default behavior and and an empty default value have a nice symmetry to them, and it allows for future change of what the default sentinel value is without breaking consuming code.
  • Since it's never modified anywhere, str should be passed by constant reference, not reference.
  • If you were going to copy the string (don't! memory allocation is both unnecessary here and is very slow compared to the rest of the operations we're using), I would take the string by value and modify the parameter. That would allow for moving in C++11, and it would conveniently handle the memory allocation for you. The (potentially major) downside to this approach would be that obviously invalid strings could cause a rather large copy. Imagine if you passed a string of pure whitespace. All that whitespace would be copied for nothing other than to get immediately removed.
  • We both missed the case of an empty string. strtol output can be used to detect it (begp == endp), but neither of us checked for that :).

I still disagree with your decision to hide the ability to check for errors, but if that's the route you want to take, I would do something like this:

int string_to_int(const std::string& str, const int& DefaultValue) {
    errno = 0;

    const char* str_beg = str.c_str();
    const char* str_end = str_beg + str.size();
    char* p = NULL;

    const int result = std::strtol(str.c_str(), &p, 10);

    if (p == str_beg) { return DefaultValue; }

    for (; p != str_end && std::isspace(*p); ++p) { /* eat trailing spaces */}

    if (p != str_end || errno != 0) {
        return DefaultValue;
    }

    return result;
}

(This could probably be optimized quite a bit, but it would likely be compiler/system specific, and it would be tedious.)

\$\endgroup\$
  • \$\begingroup\$ Hello @Corbin, thanks for sticking with me! Regarding the name: stringtoIntDef was stringToIntDef to contrast with the Delphi function StrToIntDef because it uses strings as opposed to VCL Strings, but got mangled along the way. Your method of swallowing the trailing spaces is more elegant and doesn't require the copy (ie faster), BUT it fails on a string of spaces. Not sure how to remedy that. I've added a test for zero length after stepping backwards which fixes my null string problem, and made some of your other suggested edits. \$\endgroup\$ – marcp Nov 15 '13 at 22:37
  • \$\begingroup\$ I don't understand your comment about cstdlib, if strtol is defined as part of stdlib, which is always included (in C++Builder, anyway) why would I insist on using the STL version? Are they different somehow? \$\endgroup\$ – marcp Nov 15 '13 at 22:39
  • \$\begingroup\$ Back to the default value 'discussion', when I return iDUMMY (-2^31) I am in fact putting a NULL into the database, that's what the DB engine uses in this case. If I want to capture the error I just test the result for IDUMMY, but in most cases the NULL result is the most appropriate outcome for me. \$\endgroup\$ – marcp Nov 15 '13 at 22:49
  • 1
    \$\begingroup\$ @marcp I have updated my function to work with strings of spaces. cstdlib and std::strtol aren't the STL version (or more correctly the C++ standard library). They're just the C++ version. C++ makes a point to encapsulate as much as possible in the namespace std. stdlib.h puts names in the global namespace. cstdlib puts them in std. That's the only difference. My point though was that if you include stdlib.h strtol is correct. If you include cstdlib though, std::strtol is correct. As it's C++, the namespaced cstdlib should typically be preferred. \$\endgroup\$ – Corbin Nov 15 '13 at 23:13
  • \$\begingroup\$ As for the default: Your approach is fine for your specific use case. What if in the future though -2^31 is a valid integer to parse? Then suddenly you've lost your ability to detect errors. I feel that code should do what you want it to do, not a close approximation of what you want it to do. "Try to parse an integer, or use a null value if that fails," and "Try to parse an integer, default to -2^31. If the parsed value is -2^31, pretend it's NULL" are very close for most situations, but they're not the same. It seems to fit your situation though (though I'm still skeptical of emulating null) \$\endgroup\$ – Corbin Nov 15 '13 at 23:18
8
\$\begingroup\$

There's a few things that jump out at me both in your implementation and standard options you missed. A few (scattered) thoughts follow.


Returning a default if you care about error checking doesn't make sense. How would you determine if conversion failed if there's not an integer available for use as a sentinel? You are basically doing the same thing atoi does except your default is flexible instead of 0.


Have you considered [strtol][1]? It's basically atoi with error checking capabilities. It's the fastest string to int converter you're likely going to get:

long string_to_long(const std::string& str, long default_value, int base = 10) {
    char* parse_end = NULL; //nullptr if C++11, though really you don't have to initialize this
                            //since strol is guaranteed to initialize it. I just like to avoid
                            //unitialized variables.
    long val = strtol(str.c_str(), &parse_end, base);
    if (parse_end == str.c_str() + str.size()) {
        return val;
    } else {
        return default_value;
    }
}

Note: strtol ignores leading whitespace. The function I've written above errors for trailing whitespace (or trailing anything). In other words, " 3" is fine, but "3 " would error.


The idiomatic way to do this in C++ would be to use a string stream:

int string_to_int(const std::string& str, int def)
{
    std::istringstream ss(str);
    int i;
    if (!(ss >> std::noskipws >> i)) {
        //Extracting an int failed
        return def;
    }
    char c;
    if (ss >> c) {
        //There was something after the int
        return def;
    }
    return i;
}

In terms of performance, strtol will likely be faster than this. This does have a useful advantage though that it can be made highly generic with simple templating (this is the essential idea behind boost::lexical_cast actually, but this doesn't use exceptions):

template<typename ResultType>
ResultType lexical_cast(const std::string& str, const ResultType& default_value = ResultType()) {
    std::istringstream ss(str);
    ResultType result;
    if (!(ss >> std::noskipws >> result)) {
        return default_value;
    }
    char c;
    if (ss >> c) {
        return default_value;
    }
    return result;
}

If you don't want to use exceptions, but you want to be able to detect errors, you're going to have to use a flag. You basically have two options of how to handle that. You can use a boolean flag and return the value, or you can use a boolean reference parameter and return the value. Which route you take really depends on how you want to use the code, but I would probably return the value. That lets you ignore errors if you want to, but it still doesn't force you to. I might do something like this:

template<typename ResultType>
ResultType lexical_cast(const std::string& str, bool& success) {
    std::istringstream ss(str);
    ResultType result;
    success = true;
    if (!(ss >> std::noskipws >> result)) {
        success = false;
    }
    char c;
    if (ss >> c) {
        success = false;
    }
    return result;
}

template<typename ResultType>
ResultType lexical_cast(const std::string& str) {
    bool ignore;
    return lexical_cast<ResultType>(str, ignore);
}

This gives you both options of detecting an error or ignoring one. You could even build more on top of this to have one that returns a default value:

template<typename ResultType>
ResultType lexical_cast(const std::string& str, const ResultType& default_value = ResultType()) {
    bool success;
    ResultType val(lexical_cast<ResultType>(str, ignore));
    if (success) {
        return val;
    } else {
        return default_value;
    }
}

Don't check for classes of characters manually. Just use the cctype header's functions (isspace, isdigit, etc).


You loop is way over complicated:

bool all_digit = true;
for (std::string::size_type i = 0, l = str.size(); i < l; ++i) {
    if (!std::isdigit(str[i])) { 
       all_digit = false;
        break;
    }
}

if (all_digit) { ... }

Or, allowing leading/trailing whitespace:

std::string::size_t i = 0;
const std::string::size_t len = str.size();
for (; i < len && std::isspace(str[i]); ++i) { /* empty */ }
for (; i < len && std::isdigit(str[i]); ++i) { /* empty */ }
for (; i < len && std::isspace(str[i]); ++i) { /* empty */ }
const bool all_digits = (i == len);
\$\endgroup\$
  • \$\begingroup\$ my loop is more complicated because its doing more. Your method doesn't handle leading and trailing spaces and it doesn't handle negatives. But I like the isdigit shortcut. strtol is more promising. I have included another version of the function in a separate answer using strtol. \$\endgroup\$ – marcp Nov 15 '13 at 4:16
  • \$\begingroup\$ Thanks @Corbin, While I was writing my response you were editing your answer... The strtol based function is excellent. I came up with something similar. It doesn't work for trailing whitespace, but it handles overflow (or can). Performance wise its the same as mine and IntToStrDef. Only after a million iterations can I see a difference and then the Pascal function is still fastest! My version doesn't handle overflow, I noticed. And IntToStrDef doesn't handle trailing whitespace either. Historically it did. \$\endgroup\$ – marcp Nov 15 '13 at 4:28
  • \$\begingroup\$ Regarding the use of a default return value, its very useful as a flag depending on the known context. My main use for it though is to prepare data for entry in a database. My database stores dummy values in integer channels as -2147483647, so I use that as the default and store invalid data as Dummies. \$\endgroup\$ – marcp Nov 15 '13 at 4:35
  • \$\begingroup\$ @marcp Seems i misunderstood your function's functionality. I had assumed that leading and trailing whitespace were not considered valid components of integers. In particular, I assumed only 1 or more digits were valid. All of my example functions follow that logic. I will add a snippet to check for just digits or leading/trailing whitespace. Is trailing whitespace valid or not, by the way? \$\endgroup\$ – Corbin Nov 15 '13 at 5:11
  • \$\begingroup\$ @marcp It sounds like that where you're storing -2^31 you might should be storing null. That sounds like a bit of an abuse of an integer field (hard to say without more context though). In your particular instance, a non-valid value might exist, but in the general case, it's not safe to assume (and CR is all about the general case :)). At the end of the day though, whatever works works. I would probably still go the route of implementing a defaulting one in terms of an error checking one though. \$\endgroup\$ – Corbin Nov 15 '13 at 5:14
3
\$\begingroup\$

As per @retailcoder's suggestion I'm adding the bulletproof version as a separate answer rather than edit @Corbin's answer.

This function meets all the requirements and is quite a bit cleaner than my original. It is based on @Corbin's final suggestions.

int stringtoIntDef(const std::string & str, const int & DefaultValue) {
   int result = DefaultValue;
   std::size_t i = 0;
   const std::size_t len = str.size();
   for (; i < len && std::isspace(str[i]); ++i) { /* leading space */}
   if (str[i]=='-') ++i; /* allow negative symbol */
   if (i==len) { return result; } /* no digits */
   for (; i < len && std::isdigit(str[i]); ++i) { /* digits */}
   for (; i < len && std::isspace(str[i]); ++i) { /* trailing space */}
   if (i == len) {
      errno = 0;
      const char *s = str.c_str();
      char * p;
      result = strtol(s, &p, 0);
      if (errno != 0) {
         result = DefaultValue;
      }
   }
   return result;
}

Interestingly it remains a fair bit slower than the Delphi function. Some sample results using GetTickCount(), which has a granularity of about 16, and 1 million iterations:

             str  time   ok  str time  ok   str time ok   str time ok   str time ok    str time ok
Delphi       -25    0   yes  12    16  yes  box  15  yes  " "  15  yes  inf  16  yes   "2 "  16  no
My original  -25   140  yes  12   109  yes  box  31  yes  " "  109  no  inf 375  no    "2 "  78  yes
Final        -25   250  yes  12   234  yes  box  93  yes  " "  63  yes  inf  561 yes   "2 "  249 yes

where inf is some number large enough to cause overflow.

Admittedly none of those times is very big for 1 million of anything. I just think its interesting that the Delphi function is SO efficient.

\$\endgroup\$
  • \$\begingroup\$ Glad to see you managed to extract some use out of my incoherent ramblings :). As for the performance difference.... That's not surprising. The direct conversion function only requires one linear scan as it validates while it converts. This one requires two since it does validation then passes it to strtol for another linear pass. I would essentially expect this to take twice the speed in the typical case of a valid number. \$\endgroup\$ – Corbin Nov 15 '13 at 17:31
  • \$\begingroup\$ If you were willing to disallow trailing space (as Delphi does) then you could actually use strtol directly and save a pass (you would have to explicitly use base 10 to strtol or it would allow octal/hex). \$\endgroup\$ – Corbin Nov 15 '13 at 17:32
0
\$\begingroup\$

Based on @Corbin's last comment I've created another version that seems to be a bit faster and cleaner. I just strip any whitespace off the end before calling strtol. It also has the benefit of handling numbers with a different base, if I want it to.

int stringtoIntDef(const std::string & str, const int & DefaultValue) {
   // this function was developed iteratively on Codereview.stackexchange
   // with the assistance of @Corbin
   std::size_t len = str.size();
   while (std::isspace(str[len-1]))
       len--;
   if (len==0) return DefaultValue;    
   errno = 0;
   char *s = new char[len + 1];
   std::strncpy(s, str.c_str(), len);
   char * p;
   int result = strtol(s, &p, 0);
   if ((*p!= '\0') || (errno != 0)) {
      return DefaultValue;
   }
   delete s;
   return result;
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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