Grabs 10 bytes and converts it into a hex formatted string

Question

I'm working on some code right now where the goal is to take in a byte stream and grab the data from the body of a message. The data is 10 bytes long and is meant to be translated into hex. Specifically, the 10 bytes coming in look like 01000200030004000500 or 01010201030104010501 (with implied 0x in front of each).

The code does this, but in my opinion it's not very pretty, so I was hoping to get help in improving both my own code and learning about a more effective way of grabbing an "uneven" amount of bytes (i.e. not the size of a data type for an easy memcpy). I've seen suggestions for using an array, but I wasn't sure if memcopying or endianness changes would complicate its use.

case MyMessage:
{
  std::string s;
  unsigned int i,j;
  // bytes are of type unsigned char*
  // each hex value is 4 bits
  memcpy(&i, bytes, sizeof(i)); // Bytes 0, 1, 2, 3
  i = ntohl(i); // 01000200 or 01010201

  memcpy(&j, bytes + 4, sizeof(j)); // Bytes 4, 5, 6, 7
  j = ntohl(j); // 03000400 or 03010401

  unsigned short k;
  memcpy(&k, bytes + 8, sizeof(k)); // Bytes 8, 9
  k = ntohs(k); // 0500 or 0501

  std::ostringstream iS, jS, kS;
  iS << std::hex << (i);
  s += "0" + iS.str(); // Have to shove "0" at front when dropped

  jS << std::hex << (j);
  s += "0" + jS.str();

  kS << std::hex << (k);
  s += "0" + kS.str();

  // In my actual code, these strings are constants 
  if (s == "01000200030004000500") 
  {
      s = "Off";
  }

  else if (s == "01010201030104010501")
  {
      s = "On";
  }

  else
  {
      s = "ERROR";
  }

  return s;
}

Answer 1

It looks like you overcomplicate things. I don't see why do you need to build the string. You only use it for comparison (and immediately kill it after). Comparing integers is much simpler, especially that your bytes follow nice structure. Inspect bytes one by one, along the lines of (I might be getting even/odd bytes wrong, follow their wire order):

    if ((bytes[1] != 0) && (bytes[1] != 1)) {
        return "Error";
    }

    for (int i = 0; i < 10; i += 2) {
        if (bytes[i] != i) {
            return "Error";
        }
        if (bytes[i + 1] != bytes[1]) {
            return "Error";
        }
    }

    return bytes[1] == 0? "Off": "On";

If the message structure is coincidental, and you don't have such an algorithmic approach handy, I recommend to format expected messages as byte arrays, e.g.

    char off_msg[] = {0x01, 0x00, 0x02, 0x00, 0x03, 0x00, 0x04, 0x00, 0x05, 0x00};

and memcmp them against bytes. Again, follow the wire byte order.