Fixed and improved HtmlColor::ToNumericalString

[Bradan]

The method didn't work correctly as it decremented bufLen twice and forgot one digit. Besides this, it always returned 7 instead of the actual amount of characters used.

# + 123456 + null termination = 8 chars

You can try it on your desktop computer with that code and a C++ compiler:

#include <iostream>
#include <cstdint>

class HtmlColor {
public:
    HtmlColor(uint32_t color) : Color(color) {}

    size_t ToNumericalStringOrig(char *buf, size_t bufSize) const;
    size_t ToNumericalString(char *buf, size_t bufSize) const;

private:
    uint32_t Color;
};

static inline char hexdigit(uint8_t v) {
  return v + (v < 10 ? '0' : 'a' - 10);
}

size_t HtmlColor::ToNumericalStringOrig(char* buf, size_t bufSize) const
{
  size_t bufLen = bufSize - 1;

  if (bufLen-- > 0)
  {
    if (bufLen > 0)
    {
      buf[0] = '#';
    }

    uint32_t color = Color;
    for (uint8_t indexDigit = 6; indexDigit > 0; indexDigit--)
    {
      if (bufLen > indexDigit)
      {
        buf[indexDigit] = hexdigit(color & 0x0000000f);
      }
      color >>= 4;
    }

    buf[(bufLen < 7 ? bufLen : 7)] = 0;
  }
  return 7;
}

/**
 * Generates a Html encoded hex color string (#aabbcc) with null termination.
 *
 * @param buf the buffer to write the string to
 * @param bufSize the maximum buffer size (8 recommended for # + 123456 + null terminator)
 * @return The amount of chars written to buf.
 */
size_t HtmlColor::ToNumericalString(char *buf, size_t bufSize) const {
  if (bufSize > 0) {
    buf[0] = '#';

    uint32_t color = Color;
    for (uint8_t indexDigit = 6; indexDigit > 0; --indexDigit) {
      if (bufSize > indexDigit) {
        buf[indexDigit] = hexdigit(color & 0x0000000f);
      }
      color >>= 4;
    }

    size_t lastIndex = (bufSize < 7 ? bufSize - 1 : 7);
    buf[lastIndex] = 0;

    return lastIndex+1;
  }
  return 0;
}

int main() {
  char string[8]; // # + 123456 + null terminator = 8

  HtmlColor color(0xaabbcc);

  size_t len;

  len = color.ToNumericalStringOrig(string, 8);
  std::cout << "Original result:" << std::endl << string << std::endl << len << std::endl;

  len = color.ToNumericalString(string, 8);
  std::cout << "New result:" << std::endl << string << std::endl << len << std::endl;

  return 0;
}

Output:

Original result:
#aabbc
7
New result:
#aabbcc
8

[Bradan]

Thanks for the feedback, I'm working on it.

[Bradan]

I've applied the changes and I hope everything is fine now. Do you have a style checker or formatter settings for CLion or other IDEs?

The conditional initial shift in front of the loop necessary because of the order we look at color (LSB/right side of the hex digits first), otherwise if bufSize was smaller than 8 we would have a problem. Sorry for the inconveniences with my initial commit.

[Makuna]

I've applied the changes and I hope everything is fine now. Do you have a style checker or formatter settings for CLion or other IDEs? Sorry for the inconveniences with my initial commit.

I never setup a style checker. Others submitting code is rare and my editor's defaults does most of it for me. I am not super critical and let the small stuff through. Most important is good naming and readability of code. I tend to mention it more when there is other changes blocking the merge that need to be done.

Don't worry about cycling (submit, review, revisit, review, etc) with code reviews. No one is perfect and it's a way for growth and code improvement having two eyes/brains on the topic. I try not to take any comments personal, and I hope my comments don't come off as over critical. It's just how I was trained.

If I don't review or just plain reject is when I don't think there is merit to changes and I will usually comment as to why.

This area you spotted is ripe for improvement.