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:

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:

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.

There are no strings contructed, 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).

So the question is 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 loop
• same as above, except throw Exception (which at least lets user know something went wrong)
• just document method to tell user that if values are bad, behavior is "undefined" (I have seen some methods documented this way).

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.:

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:

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.

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).

Some background on the problem: Our application has data read from a socket stored in a character array, which was a fixed length message header, containing various fixed length fields i.e:

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

int atoi(char* string, int start, int length)
{
    int result = 0;
    char* index = string + start*sizeof(char);
    for(int i=length;i>0; i--)
    {
            // remove worse implementation based on William Morris's comment
        //result += (*index++ - '0') * pow((float) 10, (float) (i-1));
            result = result * 10 + (*index++ - '0');
 
    }
    return result;
}

UPDATE

Based on critique from William Morris, and reading Apple's implementation of strol and also another atoi implementation, I realized the computation was needlessly complex, and edited it above. As far as error behavior, I see the doc for strtol gives specific return values for error conditions, so perhaps that would be the best approach.

Also, my code doesn't handle negative numbers. If this is intended to be an all-purpose library function, it needs to handle the '-' and '+' character. That's an issue to consider.

Perhaps the conclusion to draw from this, is that since this function is designed to be invoked from a class which wraps a fixed character array, the design should be scoped only for this use case (a fixed length array, where the start/end positions of each substring are fixed), and all-purpose cases should use the standard C library functions.

UPDATE 3

Revised version to review, after correcting mistake if (length > sizeof(unsigned int)) (previous version). It feels slightly insane, because I wanted a compile time constant for the number of digits in unsigned int, and compile time constants (in pre-C++ 11) are not straightforward. Here is the unit tested version, which I think is now correct.

// utils.h
namespace ezx {
    namespace str_utils {
        // use template recursion to force compiler to generate the constant
        template<unsigned int number> struct static_total_digits
        {
            static const size_t digits = 1 + static_total_digits<number / 10>::digits;
        };
        // specialized template to stop the recursion    
        template<> struct static_total_digits<0>
        {
            static const size_t digits = 0;
        };

        // make the code a little nicer to read
        typedef static_total_digits<UINT_MAX> MAX_UINT;
    
        /**
        *  Converts an array of chars (bytes) to a positive integer, using their ASCII values to determine
        *  the value of each digit. This is not designed to handle negative numbers. If the string represents 
        *  a number larger than UINT_MAX, then UINT_MAX is returned. If either start or length arguments are 
        *  outside the bounds of the array, the digits is undefined.
        *           
        *  @param string pointer to character array
        *  @start position in array to start reading characters
        *  @length number of characters to read
        */
        inline unsigned int atoui(const char* string, size_t start, size_t length)
        {
            if (length > MAX_UINT::digits) {
                return UINT_MAX;
            }
            unsigned int digits = 0;
            const char* index = string + start;
            for(size_t i=0; i < length; i++)
            {
                digits = digits * 10 + (*index++ - '0');
            }
            return digits;
        }
    }
}

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:

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

// utils.h
namespace ezx {
    namespace str_utils {
        // use template recursion to force compiler to generate the constant
        template<unsigned int number> struct static_total_digits
        {
            static const size_t digits = 1 + static_total_digits<number / 10>::digits;
        };
        // specialized template to stop the recursion    
        template<> struct static_total_digits<0>
        {
            static const size_t digits = 0;
        };

        // make the code a little nicer to read
        typedef static_total_digits<UINT_MAX> MAX_UINT;
    
        /**
        *  Converts an array of chars (bytes) to a positive integer, using their ASCII values to determine
        *  the value of each digit. This is not designed to handle negative numbers. If the string represents 
        *  a number larger than UINT_MAX, then UINT_MAX is returned. If either start or length arguments are 
        *  outside the bounds of the array, the digits is undefined.
        *           
        *  @param string pointer to character array
        *  @start position in array to start reading characters
        *  @length number of characters to read
        */
        inline unsigned int atoui(const char* string, size_t start, size_t length)
        {
            if (length > MAX_UINT::digits) {
                return UINT_MAX;
            }
            unsigned int digits = 0;
            const char* index = string + start*sizeof(char);
            for(size_t i=0; i < length; i++)
            {
                digits = digits * 10 + (*index++ - '0');
            }
            return digits;
        }
    }
}

// utils.h
namespace ezx {
    namespace str_utils {
        // use template recursion to force compiler to generate the constant
        template<unsigned int number> struct static_total_digits
        {
            static const size_t digits = 1 + static_total_digits<number / 10>::digits;
        };
        // specialized template to stop the recursion    
        template<> struct static_total_digits<0>
        {
            static const size_t digits = 0;
        };

        // make the code a little nicer to read
        typedef static_total_digits<UINT_MAX> MAX_UINT;
    
        /**
        *  Converts an array of chars (bytes) to a positive integer, using their ASCII values to determine
        *  the value of each digit. This is not designed to handle negative numbers. If the string represents 
        *  a number larger than UINT_MAX, then UINT_MAX is returned. If either start or length arguments are 
        *  outside the bounds of the array, the digits is undefined.
        *           
        *  @param string pointer to character array
        *  @start position in array to start reading characters
        *  @length number of characters to read
        */
        inline unsigned int atoui(const char* string, size_t start, size_t length)
        {
            if (length > MAX_UINT::digits) {
                return UINT_MAX;
            }
            unsigned int digits = 0;
            const char* index = string + start;
            for(size_t i=0; i < length; i++)
            {
                digits = digits * 10 + (*index++ - '0');
            }
            return digits;
        }
    }
}