I should preface this question by saying that I'm a Java programmer working on my first C++ application, so perhaps there are certain obvious things I don't know.

Some background on the problem:

Our application had data read from a socket stored in a character array, which was a fixed length message header, containing various fixed length fields i.e.:

char[15] data;

The field positions were 0-5,6-7,8-10,11-13 (the last char is a delimiter). The class which wrapped this header offered convenience functions to retrieve the values for each field:

    int ApiHeader::msg_size(void)
        return std::atoi(std::string(data, 6).c_str());

    int ApiHeader::msg_type(void)
        return std::atoi(std::string(data,6,2).c_str());

    int ApiHeader::msg_subtype(void)
        return std::atoi(std::string(data,8,3).c_str());

    int ApiHeader::seqno(void)
        return std::atoi(std::string(data,11,3).c_str());

I was trying to leverage the std::atoi which requires a c-string, but I needed to "slice" the character array into the sub-arrays which contained each separate field, which I did by constructing a string built from the positions in the data array for each field.

The code worked fine, but a user who was doing some code analysis pointed that the msg_subtype() function (and the 2 other similar ones), were actually unintentionally creating an extra string object.

They were invoking this constructor:

basic_string(const basic_string& other, 
              size_type pos, 
              size_type count = std::basic_string::npos, 
              const Allocator& alloc = Allocator() );

So the code was first constructing a new string to turn the char[] to a string&, and then it was constructing another string invoking the string(string&, int, int) constructor.

I redesigned the code to be more efficient:

int atoi(char* string, int start, int length)
    double result = 0;
    char* index = string + start*sizeof(char);
    for(int i=length;i>0; i--)
        result += (*index++ - '0') * pow((float) 10, (float) (i-1));
    return (int) result;

There are no strings constructed, and it just iterates the part of the data array specified. The problem is that this code requires that there is no way to detect whether the user passed bad values for start and length. (I'm not exactly sure what happens if user passes a length outside of the array. In my unit test, I just saw it got an uninitialized value).

What can be done to make this code more exception proof?

Some ideas I have are:

  • If *index is not within valid range ('0' to '9'), break out of the loop.
  • Same as above, except throw an Exception (which at least lets user know something went wrong).
  • Just document the method to tell user that if values are bad, behavior is "undefined" (I have seen some methods documented this way).

What would be the best C++ way of handling this issue? Or is there a completely different approach which should be used for this function?

  • \$\begingroup\$ Do you have to call these functions multiple times for a single header? Have you considered having a couple of int variables in your ApiHeader class (assuming it's a class and not a namespace). That way, you only have to do the work of extracting from the array once. \$\endgroup\$
    – red_eight
    Commented Dec 27, 2013 at 22:34
  • \$\begingroup\$ These functions are only called once per message. The data array is the header for a message. The socket reads the bytes directly into that array. \$\endgroup\$ Commented Dec 28, 2013 at 23:16
  • \$\begingroup\$ In your 2nd update the test length > sizeof(unsigned int) is invalid. You are comparing the length of the ascii representation with the size of the binary result. Also note that sizeof(char) is 1 by definition \$\endgroup\$ Commented Dec 30, 2013 at 20:32

1 Answer 1


First a question: why do you care that an extra string is being created? Is this function used a lot. If not, then I see no need to change.

But beyond that, although I don't know about C++ cleverness, is your replacement function so much more efficient? You are calling pow repeatedly and using floating point to do an integer job. Here is an alternative that would be cheaper. It returns 0 on success, 1 on failure (bad string) and the number desired is output in result. Notice that string is const.

int my_atoi(const char* string, int start, int length, long *result)
    char copy[length+1];
    memcpy(copy, string + start, length);
    copy[length] = '\0';
    char *end;
    *result = strtol(copy, &end, 0);
    return (*end != '\0');

You might also omit the start parameter and call with

my_atoi(string+start, length, &res);
  • 2
    \$\begingroup\$ Rather than return true/false. I would make it return a pointer one past the last character read. The function can then be used to parse integers out of the middle of a string. If they also want to test that all characters have been used they just de-reference the pointer. \$\endgroup\$ Commented Dec 28, 2013 at 21:09
  • \$\begingroup\$ VLAs are not legal in C++. \$\endgroup\$
    – Ben Voigt
    Commented Apr 22, 2018 at 3:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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