1
\$\begingroup\$

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;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Hi. Welcome to Code Review! It's easier to review runnable programs rather than excerpts. As it is, we'd have to write a wrapper to run the code and test changes. The simplest thing is if you can just post the entire program. But if that's not feasible, please write a wrapper that demonstrates and tests the code. \$\endgroup\$ – mdfst13 Feb 3 '16 at 18:54
  • 1
    \$\begingroup\$ Not sure what you mean by 10 hex bytes. A bytes is usually 8 bits. 8 bits can be encoded with 2 hex digits. \$\endgroup\$ – Martin York Feb 3 '16 at 20:59
1
\$\begingroup\$

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.

| improve this answer | |
\$\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.