3
\$\begingroup\$

I am writing a small game and as a part of it I load json config with colors definitions in a format of strings #00ff00.

Then I am using this function to convert these strings to SDL Color structure. I am assuming that all color strings are a valid hex colors (between 6-7 chars, depending if # (hash) is provided at the beginning).

template <class T>
inline unsigned int toIntFromHexString(const T& t) {

    unsigned int x;
    std::stringstream ss;
    ss << std::hex << t;
    ss >> x;
    return x;

}


inline SDL_Color stringToSDLColor(std::string colorString) {

    SDL_Color color;

    if (colorString[0]=='#') {
        colorString.erase(0,1);
    }

    std::string stringR = colorString.substr(0,2);
    std::string stringG = colorString.substr(2,2);
    std::string stringB = colorString.substr(4,2);
    std::string stringA = colorString.substr(6,2);

    color.r = toIntFromHexString(stringR);
    color.g = toIntFromHexString(stringG);
    color.b = toIntFromHexString(stringB);
    if (stringA.length()>0) {
        color.a = toIntFromHexString(stringA);
    }

    return color;

}

What can be adjusted in the implementation?

\$\endgroup\$
3
  • \$\begingroup\$ Is it safe to assume 6 hexadecimal digits? CSS, for instance, supports 3-digit RGB values as well, so you might consider using the length to determine your parsing. \$\endgroup\$ Aug 10 '17 at 9:29
  • \$\begingroup\$ @TobySpeight Yes, I am the one who enters the colors into the configs, so I just assume that they are all in the same format, if not then the json parser will just thrown an error. \$\endgroup\$ Aug 11 '17 at 7:31
  • \$\begingroup\$ Thanks for the clarification - it really makes a difference if the inputs are under your own control! \$\endgroup\$ Aug 11 '17 at 7:49
2
\$\begingroup\$

You could use a regex.
See the question How to identify a given string is hex color format on Stack Overflow.

#include <iostream>
#include <regex>

int main()
{
    std::string rgbcolor = "Fe23aC";

    // Not a regex expert - I couldn't get the first pattern to work

    //std::regex pattern("#?([0-9a-fA-F]{2}){3}");
    std::regex pattern("#?([0-9a-fA-F]{2})([0-9a-fA-F]{2})([0-9a-fA-F]{2})");

    std::smatch match;
    if (std::regex_match(rgbcolor, match, pattern))
    {
        // From kraskevich's comment
        auto r = std::stoul(match[1].str(), nullptr, 16);
        auto g = std::stoul(match[2].str(), nullptr, 16);
        auto b = std::stoul(match[3].str(), nullptr, 16);

        std::cout << rgbcolor << ": r = " << r << ", g = " << g << ", b = " <<
            b << "\n";
    }
    else
    {
        std::cout << rgbcolor << " is an invalid rgb color\n";
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ I may use this snippet in the future, but for the current project I am assuming that all strings contain valid hex colors. \$\endgroup\$ Aug 9 '17 at 22:54
4
\$\begingroup\$

Rather than breaking the string into substrings, then converting each individually from a hex string to an integer, I'd probably convert the entire string at once, then use bit shifting to get the pieces you care about.

SDL_Color hex2sdl(std::string input) {

    if (input[0] == '#')
        input.erase(0, 1);

    unsigned long value = stoul(input, nullptr, 16);

    SDL_Color color;

    color.a = (value >> 24) & 0xff;
    color.r = (value >> 16) & 0xff;
    color.g = (value >> 8) & 0xff;
    color.b = (value >> 0) & 0xff;
    return color;
}

This simplifies the code quite a bit, and at least in my mind makes it rather easier to read.

Another point to consider (when/if your compiler supports it) would be to use an std::string_view instead of a std::string for the input. A string_view is something like a pointer/length, so (when needed) it can trim the # from the beginning without copying all the data like an actual string normally will. Even if your compiler supports string_view itself, however, it may easily not support passing a string_view to std::stoul, which kind of ruins the idea (for now).

\$\endgroup\$
2
  • \$\begingroup\$ Did stoxx functions get string_view versions? Erasing at the beginning looks like a lot of unnecessary work, we could just increment the start. \$\endgroup\$ Aug 10 '17 at 5:41
  • \$\begingroup\$ @Incomputable: no the stoxxx don't change (AFAIK). Instead, they're adding from_chars, which take pointers to the beginning/end of the data to convert. open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0067r1.html. To be entirely honest, I think the current proposal could use some polishing. If nothing else, it should allow arbitrary iterators instead of only pointers, and when ranges get settled, it should support ranges as well. \$\endgroup\$ Aug 11 '17 at 17:09
2
\$\begingroup\$
  1. You can write simpler code without stringstream's here:
    std::stoul(t, nullptr, 16) converts a string t to an unsigned int (the last argument is the base).

  2. I think that !stringA.empty() expresses the intent more clearly than
    stringA.length() > 0.

\$\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.