2
\$\begingroup\$

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?

\$\endgroup\$
  • \$\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 Dec 27 '13 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\$ – Sam Goldberg Dec 28 '13 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\$ – William Morris Dec 30 '13 at 20:32
1
\$\begingroup\$

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);
\$\endgroup\$
  • 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\$ – Martin York Dec 28 '13 at 21:09
  • \$\begingroup\$ VLAs are not legal in C++. \$\endgroup\$ – Ben Voigt Apr 22 '18 at 3:02

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.