current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 1 down vote favorite

I wrote this function to convert an array of bytes to a C/C++ string representation using hexadecimal escape codes (\xhh). Any suggestions are welcome:

std::string toEscapedHexaString(const std::uint8_t * data, const int dataSizeBytes, 
                                const int maxCols = 80, const int padding = 0)
{
    assert(data != nullptr);
    assert((maxCols % 4) == 0);
    assert((padding % 2) == 0);

    int column = 0;
    char hexaStr[64] = {'\0'};

    std::string result = "\"";
    for (int i = 0; i < dataSizeBytes; ++i, ++data)
    {
        std::snprintf(hexaStr, sizeof(hexaStr), "\\x%02X", static_cast<unsigned>(*data));
        result += hexaStr;
        column += 4;

        if (column >= maxCols)
        {
            if (i != (dataSizeBytes - 1)) // If not the last iteration
            {
                result += "\"\n\"";
            }
            column = 0;
        }
    }

    // Add zero padding at the end to ensure the data size
    // is evenly divisible by the given padding value.
    if (padding > 0)
    {
        for (int i = dataSizeBytes; (i % padding) != 0; ++i)
        {
            result += "\\x00";
            column += 4;

            if (column >= maxCols)
            {
                if ((i + 1) % padding) // If not the last iteration
                {
                    result += "\"\n\"";
                }
                column = 0;
            }
        }
    }

    result += "\"";
    return result;
}

You can control the number of columns in the output, so for example, converting Hello World!\0 with 20 columns max (not counting the quotes):

const unsigned char str[] = "Hello World!";
auto result = toEscapedHexaString(str, sizeof(str), 20);

/* result = 
   "\x48\x65\x6C\x6C\x6F"
   "\x20\x57\x6F\x72\x6C"
   "\x64\x21\x00"
*/
share|improve this question
up vote 1 down vote accepted

Use of assert

Some of the uses of assert here look semi-broken, at least to me. Part of the definition of assert is that when you compile with NDEBUG defined, assert becomes a nop. When it is enabled, a failed assertion immediately aborts the program. For testing inputs to a function, neither of those is generally desirable. For such purposes, I usually define an assure that always does its thing, and throws an exception when the test fails.

snprintf

At the very least, I'd try to wrap the ugliness of using snprintf up into a reasonably neat little function like to_hex that just converts a number into its hex representation:

std::string to_hex(int in) { 
    // snprintf ugliness here
    return std::string(buffer);
}

Alternatively, since you're converting inputs to strings, and accumulating those together into a buffer anyway, consider using an std::ostringstream.

Logic

In a few places, the logic seems...less than pleasing, at least to me. For example:

if (column >= maxCols)

You've previously tested that maxCols % 4 == 0, and (assuming things work correctly) each item you add should be 4 more characters, so you should never see a result that's actually greater than maxCols--only less than maxCols, then equal to maxCols, then back to 0. This is harmless, and if you might want to support a maxCols that was not a multiple of 4, then it might even make good sense--but as things stand right now, it seems like there's a degree of...uncertainty about the real intent.

Reorganization

Right now you have code to ensure the maximum line length sprinkled liberally throughout the rest of the code. I'd be tempted to define a type that takes that as its single responsibility: when you create it, specify a maximum length. When you add characters to it, it tracks the length of the current "line", and when the maximum is reached (or exceeded) it adds a delimiter, and starts over. With this, the rest of the code gets a lot simpler in a hurry, and each piece of code has a much simpler, clearer purpose.

share|improve this answer
    
Thanks Jerry, good points! I think you meant to_string instead? stoi is for the other way around, string => integer? Unfortunately to_string doesn't allow specifying a base, so I'm thinking the only other way to avoid snprintf is using a stringstream... – glampert Feb 18 at 3:11
    
Oops-yes, stoi was the wrong one. But you're right-I'd forgotten to_string doesn't let you specify the base. – Jerry Coffin Feb 18 at 3:44

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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